[JSC] Add Air lowering for BitOr and impove BitAnd

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

[JSC] Add Air lowering for BitOr and impove BitAnd

[Benjamin Poulain]

Created attachment 264650 [details]
Patch

[Filip Pizlo]

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.

[Filip Pizlo]

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

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Created attachment 264668 [details]
Patch

[Benjamin Poulain]

Created attachment 264735 [details]
Patch

[Benjamin Poulain]

Comment on attachment 264735 [details]
Patch

Clearing flags on attachment: 264735

Committed r191983: <http://trac.webkit.org/changeset/191983>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.