Bug 207330

Summary: Fix code origin when lowering offlineasm instructions on MIPS/ARM64E
Product: WebKit Reporter: Angelos Oikonomopoulos <angelos>
Component: New BugsAssignee: Angelos Oikonomopoulos <angelos>
Status: RESOLVED FIXED    
Severity: Normal CC: clopez, ddkilzer, ews-watchlist, guijemont, keith_miller, mark.lam, msaboff, pmatos, saam, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 210639    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Angelos Oikonomopoulos 2020-02-06 06:25:44 PST
Fix code origin when lowering offlineasm instructions on MIPS
Comment 1 Angelos Oikonomopoulos 2020-02-06 06:31:10 PST
Created attachment 389946 [details]
Patch
Comment 2 Angelos Oikonomopoulos 2020-02-06 06:36:13 PST
Created attachment 389947 [details]
Patch
Comment 3 Angelos Oikonomopoulos 2020-02-06 06:39:37 PST
Created attachment 389948 [details]
Patch
Comment 4 Angelos Oikonomopoulos 2020-03-24 08:33:23 PDT
Created attachment 394368 [details]
Patch
Comment 5 Angelos Oikonomopoulos 2020-03-24 08:34:12 PDT
Rebased and re-tested, no functional changes.
Comment 6 Angelos Oikonomopoulos 2020-04-09 03:44:57 PDT
Created attachment 395933 [details]
Patch
Comment 7 Mark Lam 2020-04-15 17:47:07 PDT
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()?
Comment 8 Angelos Oikonomopoulos 2020-04-17 03:32:50 PDT
Created attachment 396753 [details]
Patch
Comment 9 Angelos Oikonomopoulos 2020-04-17 07:11:17 PDT
Lastest patch implements the suggested naming changes. Thanks!
Comment 10 Mark Lam 2020-04-17 07:32:35 PDT
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.
Comment 11 Mark Lam 2020-04-17 08:35:43 PDT
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.
Comment 12 Angelos Oikonomopoulos 2020-04-18 07:01:25 PDT
Created attachment 396846 [details]
Patch
Comment 13 Angelos Oikonomopoulos 2020-04-18 07:03:55 PDT
(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.
Comment 14 Mark Lam 2020-04-18 07:38:17 PDT
Comment on attachment 396846 [details]
Patch

r=me
Comment 15 EWS 2020-04-18 07:59:15 PDT
Committed r260310: <https://trac.webkit.org/changeset/260310>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396846 [details].
Comment 16 Radar WebKit Bug Importer 2020-04-18 08:00:15 PDT
<rdar://problem/61971467>
Comment 17 David Kilzer (:ddkilzer) 2020-04-18 09:10:21 PDT
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.)
Comment 18 Yusuke Suzuki 2020-04-18 09:25:17 PDT
Committed r260314: <https://trac.webkit.org/changeset/260314>