Bug 150827

Created attachment 264650 [details] Patch

Comment on attachment 264650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264650&action=review This breaks the build because Patchpoints use immOrTmp. Patchpoints could actually handle a imm64, so they could just use immForMove. But that calls into question the use of the name "immForMove" if it's being used for Patchpoints. ;-) > Source/JavaScriptCore/b3/B3LowerToAir.cpp:229 > + // The MacroAssembler move with a 32bit immediate zero extend the value, losing > + // the top bits for negative values. This isn't true - the X86_64 semantics of a 32-bit immediate is sign extension. Maybe the semantics call for zero-extension in some instructions but not others? > Source/JavaScriptCore/b3/B3LowerToAir.cpp:237 > + if (value->representableAs<int64_t>()) > + return Arg::imm64(value->asNumber<int64_t>()); I don't think this belongs here, if the name of the instruction is "ForMove". Move in Air cannot do a 64-bit move to an address. And yet, all of the users of this are happy with 64-bit immediates. Also, to fix the build you need to use this exactly method for Patchpoints. That will actually fix a bug: in trunk, Patchpoints will require an extra Move to materialize 64-bit immediates, even though the Patchpoint code will admit an imm64. Oddly, Patchpoints also accept imm64's for double uses. I think there are two paths forward for fixing the Patchpoint build and making this make more sense. I don't care which we do: 1) Special-case Patchpoints, maybe with their own helpers, on the grounds that Patchpoints are unusually permissive when it comes to operands. If you do that, then all callers of immForMove or immOrTmpForMove really want to just emit a move of an immediate-or-whatever to a temporary. You could make this a lot simpler by introducing a method called moveToTmp(Value*, Tmp). It will emit a relaxedMoveForType(value->type()) and it will use whatever immediate it can. 2) Continue to have a method called immOrTmp, rather than immForMove, since this method is called for Patchpoints without actually requiring a separate Move.

Comment on attachment 264650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264650&action=review >> Source/JavaScriptCore/b3/B3LowerToAir.cpp:229 >> + // the top bits for negative values. > > This isn't true - the X86_64 semantics of a 32-bit immediate is sign extension. Maybe the semantics call for zero-extension in some instructions but not others? Actually, marking r- because I don't think we want to land this until we have clarity on this issue. We should ensure that the semantics of Arg::imm in a 64-bit context are consistent. Either it should be zero extension, or sign extension. For example, I just tested the following pieces of code and in both cases I got sign extension rather than zero extension: .text .globl _foo _foo: mov $-1, %rax ret .globl _bar _bar: add $-1, %rdi mov %rdi, %rax ret So it would seem that X86 wants to do sign extension. I believe that the "bug" here is how move(TrustedImm32, RegisterID) is implemented. It would probably be very risky to fix this bug by changing MacroAssembler behavior. But, we could introduce a second form of move(TrustedImm32, RegisterID) that does sign extension. We could call it signExtend32ToPtr(TrustedImm32, RegisterID) and we could do the following in AirOpcode.opcodes: Imm, Tmp as signExtend32ToPtr

(In reply to comment #3) > Comment on attachment 264650 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264650&action=review > > >> Source/JavaScriptCore/b3/B3LowerToAir.cpp:229 > >> + // the top bits for negative values. > > > > This isn't true - the X86_64 semantics of a 32-bit immediate is sign extension. Maybe the semantics call for zero-extension in some instructions but not others? That's a move with REX.W. If you check the x86 MacroAssembler, it does a 32bits moves. I did not change the implementation since a comment says it is intentional. I'll add a signExtend32ToPtr for a REX.W with 32bits immediate.

Created attachment 264668 [details] Patch

Created attachment 264735 [details] Patch

Comment on attachment 264735 [details] Patch Clearing flags on attachment: 264735 Committed r191983: <http://trac.webkit.org/changeset/191983>

All reviewed patches have been landed. Closing bug.