| Message ID | 1395246197.28929.38.camel@localhost.localdomain |
|---|---|
| State | New |
| Headers | show |
On 03/19/2014 04:23 PM, Edward Nevill wrote:
> I believe the shift removed by the patch below is mistaken. 'byte_offset' is already the offset in bytes so shifting it by scale of the data being loaded is wrong.
Absolutely right. It would be much less confusing if the argument passed
were a size rather than a shift, and form_address used exact_log2 to get
the shift. But your patch is OK.
Andrew.
diff -r b56e2e46bfe1 -r 9393c177ac9b src/cpu/aarch64/vm/macroAssembler_aarch64.cpp --- a/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp Wed Mar 19 10:39:35 2014 +0000 +++ b/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp Wed Mar 19 16:15:50 2014 +0000 @@ -1399,7 +1399,7 @@ } // Do it the hard way - mov(Rd, byte_offset << shift); + mov(Rd, byte_offset); add(Rd, base, Rd); return Address(Rd); } --- CUT HERE ---
Hi, I came across this while looking at the implicit exception offsets. I believe the shift removed by the patch below is mistaken. 'byte_offset' is already the offset in bytes so shifting it by scale of the data being loaded is wrong. Regards, Ed. Here is some context Address MacroAssembler::form_address(Register Rd, Register base, long byte_offset, int shift) { if (Address::offset_ok_for_immed(byte_offset, shift)) // It fits; no need for any heroics return Address(base, byte_offset); // Don't do anything clever with negative or misaligned offsets unsigned mask = (1 << shift) - 1; if (byte_offset < 0 || byte_offset & mask) { mov(Rd, byte_offset); <<<<< Here it doesn't apply the shift add(Rd, base, Rd); return Address(Rd); } // See if we can do this with two 12-bit offsets { unsigned long word_offset = byte_offset >> shift; <<<< and here it assumes it needs to shift right unsigned long masked_offset = word_offset & 0xfff000; if (Address::offset_ok_for_immed(word_offset - masked_offset) && Assembler::operand_valid_for_add_sub_immediate(masked_offset << shift)) { add(Rd, base, masked_offset << shift); word_offset -= masked_offset; return Address(Rd, word_offset << shift); } } // Do it the hard way mov(Rd, byte_offset << shift); <<<<<< but here it shift the byte offset left???? add(Rd, base, Rd); return Address(Rd); } --- CUT HERE --- exporting patch: # HG changeset patch # User Edward Nevill edward.nevill@linaro.org # Date 1395245750 0 # Wed Mar 19 16:15:50 2014 +0000 # Node ID 9393c177ac9b9407f1f4e58bd662b719b40ded54 # Parent b56e2e46bfe1de5761fbdaf4fd9b021320ab3a18 Remove mistaken shift in form_address