Bug 207330 – Fix code origin when lowering offlineasm instructions on MIPS/ARM64E

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Angelos Oikonomopoulos]

Fix code origin when lowering offlineasm instructions on MIPS

[Angelos Oikonomopoulos]

Created attachment 389946 [details]
Patch

[Angelos Oikonomopoulos]

Created attachment 389947 [details]
Patch

[Angelos Oikonomopoulos]

Created attachment 389948 [details]
Patch

[Angelos Oikonomopoulos]

Created attachment 394368 [details]
Patch

[Angelos Oikonomopoulos]

Rebased and re-tested, no functional changes.

[Angelos Oikonomopoulos]

Created attachment 395933 [details]
Patch

[Mark Lam]

Comment on attachment 395933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395933&action=review

The patch login generally looks good but the names don't describe what is actually being done.  I suggested replacement names.  r- for now because I don't think this patch is ready for landing.

> Source/JavaScriptCore/offlineasm/arm64e.rb:57
> +                    newList << Instruction.duplicate_with_operands(operands)

1. Let's use camel case for the function name because we've been using camel case everywhere else for functions that we define.
2. I suggest calling this cloneWithNewOperands() instead.  I think this is a better name because it tells us that we're effectively cloning the instruction, except that we're giving it new operands.  "duplicate" implies that we're keeping the original.  We're not.  We'll be replacing it: returning wasHandled true tells the client that it doesn't need to process this instruction and can skip it, where "processing" this instruction may mean just adding it to the newList as is.

> Source/JavaScriptCore/offlineasm/ast.rb:921
> +    def duplicate_with_operands(operands)

Let's call this cloneWithNewOperands(), and let's rename the argument to newOperands.

> Source/JavaScriptCore/offlineasm/mips.rb:560
> +    def mipsAsRegister(preList, postList, operandIndex, needRestore)

How about calling this mipsCloneWithOperandLowered()?

> Source/JavaScriptCore/offlineasm/risc.rb:431
> +def riscOperandAsRegister(insn, preList, postList, operandIndex, suffix, needStore)

The old riscAsRegister() name wasn't a very good name to begin with.  The new one isn't better.  Let's call it riscLowerOperandToRegister() instead.

> Source/JavaScriptCore/offlineasm/risc.rb:443
> +def riscOperandsAsRegisters(insn, preList, postList, suffix)

Let's call this riscLowerOperandsToRegisters().

> Source/JavaScriptCore/offlineasm/risc.rb:455
> +    def riscAsRegister(preList, postList, operandIndex, suffix, needStore)

Let's call this riscCloneWithOperandLowered() instead.

> Source/JavaScriptCore/offlineasm/risc.rb:461
> +    def riscAsRegisters(preList, postList, suffix)

Let's call this riscCloneWithOperandsLowered() instead.

> Source/JavaScriptCore/offlineasm/risc.rb:483
> +                newList << node.riscAsRegisters(newList, postInstructions, "i")

riscAsRegisters() is a bad name here.  This operation is actually doing 2 things: (1) cloning the instruction, and (2) lowering the operands to registers based on the suffix.  How about something like riscCloneWithOperandsLowered()?

> Source/JavaScriptCore/offlineasm/risc.rb:498
> +                newList << node.riscAsRegister(newList, postInstructions, 0, "p", false)

riscAsRegister() is also a bad name here.  The operation is actually (1) cloning the instruction with (2) the operand lowered to a register.  How about calling this riscCloneWithOperandLowered()?

[Angelos Oikonomopoulos]

Created attachment 396753 [details]
Patch

[Angelos Oikonomopoulos]

Lastest patch implements the suggested naming changes. Thanks!

[Mark Lam]

Comment on attachment 396753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396753&action=review

r=me with fixes.

> Source/JavaScriptCore/offlineasm/arm64e.rb:48
> +                        tag = riscLowerOperandToRegister(node, newList, postInstructions,
> +                                                    1, "p", false)

Please fix alignment or preferably, put on 1 line.

> Source/JavaScriptCore/offlineasm/risc.rb:449
> +        newOperands << riscLowerOperandToRegister(insn, preList, postList,
> +                                             index, suffix,
> +                                             index == insn.operands.size - 1)

This alignment is off.  Please fix the alignment, or preferably, just put this on 1 line.

> Source/JavaScriptCore/offlineasm/risc.rb:458
> +        operand = riscLowerOperandToRegister(self, preList, postList,
> +                                                       operandIndex, suffix,
> +                                                       needStore)

This alignment is off.  Please fix the alignment, or preferably, just put this on 1 line.

[Mark Lam]

Just a reminder that https://bugs.webkit.org/show_bug.cgi?id=210639 logically conflicts with this patch.  The changes may merge cleanly, but it will be broken unless fixed.  This patch will have to be updated because there's a new use of riscAsRegisters() added in 210639.

[Angelos Oikonomopoulos]

Created attachment 396846 [details]
Patch

[Angelos Oikonomopoulos]

(In reply to Mark Lam from comment #11)
> Just a reminder that https://bugs.webkit.org/show_bug.cgi?id=210639
> logically conflicts with this patch.  The changes may merge cleanly, but it
> will be broken unless fixed.  This patch will have to be updated because
> there's a new use of riscAsRegisters() added in 210639.

Thanks. Applied the whitespace changes and merged it with 210639.

[Mark Lam]

Comment on attachment 396846 [details]
Patch

r=me

[EWS]

Committed r260310: <https://trac.webkit.org/changeset/260310>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396846 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61971467>

[David Kilzer (:ddkilzer)]

This commit appears to have broken some of Apple's internal bots building for iOS 13.4:

    /bin/sh -c JavaScriptCore.roots/BuildRecords/JavaScriptCore_install/TempContent/Objects/JavaScriptCore.build/Offline\\\ Assembler.build/Script-65788AA018B409EB00C189FF.sh
JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/arm64e.rb:56:in `lowerMisplacedAddressesARM64E': undefined method `cloneWithNewOperands' for Instruction:Class (NoMethodError)
        from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/risc.rb:470:in `block in riscLowerMisplacedAddresses'
        from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/risc.rb:466:in `each'
        from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/risc.rb:466:in `riscLowerMisplacedAddresses'
        from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/arm64e.rb:31:in `getModifiedListARM64E'
        from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/backends.rb:189:in `lower'
        from JavaScriptCore/offlineasm/asm.rb:406:in `block (5 levels) in <main>'
        from JavaScriptCore/offlineasm/asm.rb:107:in `inAsm'
        from JavaScriptCore/offlineasm/asm.rb:405:in `block (4 levels) in <main>'
        from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/settings.rb:210:in `emitCodeInConfiguration'
        from JavaScriptCore/offlineasm/asm.rb:403:in `block (3 levels) in <main>'
        from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/settings.rb:107:in `forSettings'
        from JavaScriptCore/offlineasm/asm.rb:390:in `block (2 levels) in <main>'
        from JavaScriptCore/offlineasm/asm.rb:386:in `each'
        from JavaScriptCore/offlineasm/asm.rb:386:in `block in <main>'
        from JavaScriptCore/offlineasm/asm.rb:376:in `open'
        from JavaScriptCore/offlineasm/asm.rb:376:in `<main>'
Command PhaseScriptExecution failed with a nonzero exit code

(Apologies for the truncated paths; was necessary to sanitize the output.)

[Yusuke Suzuki]

Committed r260314: <https://trac.webkit.org/changeset/260314>