Skip to content

Commit

Permalink
Fix ImmLogic.invert(), and with it, fcopysign and float_misc test.
Browse files Browse the repository at this point in the history 
Previously, `fcopysign` was mysteriously failing to pass the
`float_misc` spec test. This was tracked down to bad logical-immediate
masks used to separate the sign and not-sign bits. In particular, the
masks for the and-not operations were wrong. The `invert()` function on
an `ImmLogic` immediate, it turns out, assumed every immediate would be
used by a 64-bit instruction; `ImmLogic` immediates are subtly different
for 32-bit instructions. This change tracks the instruction size (32 or
64 bits) intended for use with each such immediate, and passes it back
into `maybe_from_u64` when computing the inverted immediate.

Addresses one of the failures (`float_misc`) for #1521 (test failures)
and presumably helps #1519 (SpiderMonkey integration).
  • Loading branch information
@cfallin
cfallin committed Apr 23, 2020
1 parent d8920c0 commit c0df46a
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 17 deletions.
1 change: 0 additions & 1 deletion build.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,6 @@ fn should_panic(testsuite: &str, testname: &str) -> bool {
| ("spec_testsuite", "call")
| ("spec_testsuite", "conversions")
| ("spec_testsuite", "f32_bitwise")
| ("spec_testsuite", "float_misc")
| ("spec_testsuite", "i32")
| ("spec_testsuite", "i64")
| ("spec_testsuite", "int_exprs")
Expand Down
14 changes: 14 additions & 0 deletions cranelift/codegen/src/isa/aarch64/inst/args.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use crate::binemit::CodeOffset;
use crate::ir::Type;
use crate::isa::aarch64::inst::*;
use crate::isa::aarch64::lower::ty_bits;

use regalloc::{RealRegUniverse, Reg, Writable};

Expand Down Expand Up @@ -526,6 +527,19 @@ impl InstSize {
}
}

/// Convert from an integer type into the smallest size that fits.
pub fn from_ty(ty: Type) -> InstSize {
Self::from_bits(ty_bits(ty))
}

/// Convert to I32 or I64.
pub fn to_ty(self) -> Type {
match self {
InstSize::Size32 => I32,
InstSize::Size64 => I64,
}
}

pub fn sf_bit(&self) -> u32 {
match self {
InstSize::Size32 => 0,
Expand Down
50 changes: 34 additions & 16 deletions cranelift/codegen/src/isa/aarch64/inst/imms.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#[allow(dead_code)]
use crate::ir::types::*;
use crate::ir::Type;
use crate::isa::aarch64::inst::InstSize;
use crate::machinst::*;

use regalloc::RealRegUniverse;
Expand Down Expand Up @@ -233,6 +234,8 @@ pub struct ImmLogic {
pub r: u8,
/// `R` field: rotate amount.
pub s: u8,
/// Was this constructed for a 32-bit or 64-bit instruction?
pub size: InstSize,
}

impl ImmLogic {
Expand All @@ -243,6 +246,7 @@ impl ImmLogic {
if ty != I64 && ty != I32 {
return None;
}
let inst_size = InstSize::from_ty(ty);

let original_value = value;

Expand Down Expand Up @@ -423,11 +427,12 @@ impl ImmLogic {
n: out_n != 0,
r: r as u8,
s: s as u8,
size: inst_size,
})
}

pub fn from_raw(value: u64, n: bool, r: u8, s: u8) -> ImmLogic {
ImmLogic { n, r, s, value }
pub fn from_raw(value: u64, n: bool, r: u8, s: u8, size: InstSize) -> ImmLogic {
ImmLogic { n, r, s, value, size }
}

/// Returns bits ready for encoding: (N:1, R:6, S:6)
Expand All @@ -443,7 +448,7 @@ impl ImmLogic {
/// Return an immediate for the bitwise-inverted value.
pub fn invert(&self) -> ImmLogic {
// For every ImmLogical immediate, the inverse can also be encoded.
Self::maybe_from_u64(!self.value, I64).unwrap()
Self::maybe_from_u64(!self.value, self.size.to_ty()).unwrap()
}
}

Expand Down Expand Up @@ -613,7 +618,8 @@ mod test {
value: 1,
n: true,
r: 0,
s: 0
s: 0,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(1, I64)
);
Expand All @@ -623,7 +629,8 @@ mod test {
value: 2,
n: true,
r: 63,
s: 0
s: 0,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(2, I64)
);
Expand All @@ -637,7 +644,8 @@ mod test {
value: 248,
n: true,
r: 61,
s: 4
s: 4,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(248, I64)
);
Expand All @@ -649,7 +657,8 @@ mod test {
value: 1920,
n: true,
r: 57,
s: 3
s: 3,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(1920, I64)
);
Expand All @@ -659,7 +668,8 @@ mod test {
value: 0x7ffe,
n: true,
r: 63,
s: 13
s: 13,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(0x7ffe, I64)
);
Expand All @@ -669,7 +679,8 @@ mod test {
value: 0x30000,
n: true,
r: 48,
s: 1
s: 1,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(0x30000, I64)
);
Expand All @@ -679,7 +690,8 @@ mod test {
value: 0x100000,
n: true,
r: 44,
s: 0
s: 0,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(0x100000, I64)
);
Expand All @@ -689,7 +701,8 @@ mod test {
value: u64::max_value() - 1,
n: true,
r: 63,
s: 62
s: 62,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(u64::max_value() - 1, I64)
);
Expand All @@ -699,7 +712,8 @@ mod test {
value: 0xaaaaaaaaaaaaaaaa,
n: false,
r: 1,
s: 60
s: 60,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(0xaaaaaaaaaaaaaaaa, I64)
);
Expand All @@ -709,7 +723,8 @@ mod test {
value: 0x8181818181818181,
n: false,
r: 1,
s: 49
s: 49,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(0x8181818181818181, I64)
);
Expand All @@ -719,7 +734,8 @@ mod test {
value: 0xffc3ffc3ffc3ffc3,
n: false,
r: 10,
s: 43
s: 43,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(0xffc3ffc3ffc3ffc3, I64)
);
Expand All @@ -729,7 +745,8 @@ mod test {
value: 0x100000001,
n: false,
r: 0,
s: 0
s: 0,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(0x100000001, I64)
);
Expand All @@ -739,7 +756,8 @@ mod test {
value: 0x1111111111111111,
n: false,
r: 0,
s: 56
s: 56,
size: InstSize::Size64,
}),
ImmLogic::maybe_from_u64(0x1111111111111111, I64)
);
Expand Down
2 changes: 2 additions & 0 deletions cranelift/codegen/src/isa/aarch64/lower.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2110,13 +2110,15 @@ fn lower_insn_to_regs<C: LowerCtx<I = Inst>>(ctx: &mut C, insn: IRInst) {
/* n = */ false,
/* r = */ 1,
/* s = */ 0,
/* size = */ InstSize::Size32,
)
} else {
ImmLogic::from_raw(
/* value = */ 0x8000_0000_0000_0000,
/* n = */ true,
/* r = */ 1,
/* s = */ 0,
/* size = */ InstSize::Size64,
)
};
let alu_op = choose_32_64(ty, ALUOp::And32, ALUOp::And64);
Expand Down

0 comments on commit c0df46a

Please sign in to comment.