⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 285152 in webkit


Ignore:
Timestamp:
Nov 1, 2021, 8:42:34 PM (5 years ago)
Author:
Ross Kirsling
Message:

[JSC][LLInt] Non-commutative binops are hard to reason about when operands are labelled in reverse
​https://bugs.webkit.org/show_bug.cgi?id=232598

Reviewed by Saam Barati.

In offlineasm, OP a, b, c is c = a OP b but OP a, b is b = b OP a.

This can make identifiers like left and right quite confusing --
simple cases like subd left, right are already misleading, while OpDiv literally
passes its RHS to a macro as left and then checks left for division by zero.
It becomes difficult to keep this all in one's brain without rewriting it on paper.

This patch may not constitute a "complete solution", but it at least makes our naming honest:

  1. Use 3-argument syntax (as left, right, result) whenever possible.
  2. When not possible (e.g. because bsubio isn't flexible about its arguments or because x86 doesn't have 3-argument shift operations), then say rhs, lhs explicitly.
  • llint/LowLevelInterpreter32_64.asm:
  • llint/LowLevelInterpreter64.asm:
Location:
trunk/Source/JavaScriptCore
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r285149 r285152  
     12021-11-01  Ross Kirsling  <ross.kirsling@sony.com>
     2
     3        [JSC][LLInt] Non-commutative binops are hard to reason about when operands are labelled in reverse
     4        https://bugs.webkit.org/show_bug.cgi?id=232598
     5
     6        Reviewed by Saam Barati.
     7
     8        In offlineasm, `OP a, b, c` is `c = a OP b` but `OP a, b` is `b = b OP a`.
     9
     10        This can make identifiers like `left` and `right` quite confusing --
     11        simple cases like `subd left, right` are already misleading, while OpDiv literally
     12        passes its RHS to a macro as `left` and then checks `left` for division by zero.
     13        It becomes difficult to keep this all in one's brain without rewriting it on paper.
     14
     15        This patch may not constitute a "complete solution", but it at least makes our naming honest:
     16        1. Use 3-argument syntax (as `left, right, result`) whenever possible.
     17        2. When not possible (e.g. because `bsubio` isn't flexible about its arguments or
     18           because x86 doesn't have 3-argument shift operations), then say `rhs, lhs` explicitly.
     19
     20        * llint/LowLevelInterpreter32_64.asm:
     21        * llint/LowLevelInterpreter64.asm:
     22
    1232021-11-01  Yusuke Suzuki  <ysuzuki@apple.com>
    224

  • trunk/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm

    r284923 r285152  
    11191119        updateBinaryArithProfile(size, opcodeStruct, ArithProfileIntInt, t5, t2)
    11201120        get(m_dst, t2)
    1121         integerOperationAndStore(t3, t1, t0, .slow, t2)
     1121        integerOperationAndStore(t3, t0, t1, .slow, t2)
    11221122        dispatch()
    11231123
    … …  
    11361136        get(m_dst, t1)
    11371137        fii2d t0, t2, ft0
    1138         doubleOperation(ft1, ft0)
     1138        doubleOperation(ft0, ft1, ft0)
    11391139        stored ft0, [cfr, t1, 8]
    11401140        dispatch()
    … …  
    11471147        ci2ds t0, ft0
    11481148        fii2d t1, t3, ft1
    1149         doubleOperation(ft1, ft0)
     1149        doubleOperation(ft0, ft1, ft0)
    11501150        stored ft0, [cfr, t2, 8]
    11511151        dispatch()
    … …  
    11591159macro binaryOp(opcodeName, opcodeStruct, integerOperation, doubleOperation)
    11601160    binaryOpCustomStore(opcodeName, opcodeStruct,
    1161         macro (int32Tag, left, right, slow, index)
    1162             integerOperation(left, right, slow)
     1161        macro (int32Tag, lhs, rhs, slow, index)
     1162            integerOperation(lhs, rhs, slow)
    11631163            storei int32Tag, TagOffset[cfr, index, 8]
    1164             storei right, PayloadOffset[cfr, index, 8]
     1164            storei lhs, PayloadOffset[cfr, index, 8]
    11651165        end,
    11661166        doubleOperation)
    … …  
    11681168
    11691169binaryOp(add, OpAdd,
    1170     macro (left, right, slow) baddio left, right, slow end,
    1171     macro (left, right) addd left, right end)
     1170    macro (lhs, rhs, slow) baddio rhs, lhs, slow end,
     1171    macro (left, right, result) addd left, right, result end)
    11721172
    11731173
    11741174binaryOpCustomStore(mul, OpMul,
    1175     macro (int32Tag, left, right, slow, index)
     1175    macro (int32Tag, lhs, rhs, slow, index)
    11761176        const scratch = int32Tag   # We know that we can reuse the int32Tag register since it has a constant.
    1177         move right, scratch
    1178         bmulio left, scratch, slow
     1177        move lhs, scratch
     1178        bmulio rhs, scratch, slow
    11791179        btinz scratch, .done
    1180         bilt left, 0, slow
    1181         bilt right, 0, slow
     1180        bilt rhs, 0, slow
     1181        bilt lhs, 0, slow
    11821182    .done:
    11831183        storei Int32Tag, TagOffset[cfr, index, 8]
    11841184        storei scratch, PayloadOffset[cfr, index, 8]
    11851185    end,
    1186     macro (left, right) muld left, right end)
     1186    macro (left, right, result) muld left, right, result end)
    11871187
    11881188
    11891189binaryOp(sub, OpSub,
    1190     macro (left, right, slow) bsubio left, right, slow end,
    1191     macro (left, right) subd left, right end)
     1190    macro (lhs, rhs, slow) bsubio rhs, lhs, slow end,
     1191    macro (left, right, result) subd left, right, result end)
    11921192
    11931193
    11941194binaryOpCustomStore(div, OpDiv,
    1195     macro (int32Tag, left, right, slow, index)
    1196         ci2ds left, ft0
    1197         ci2ds right, ft1
     1195    macro (int32Tag, lhs, rhs, slow, index)
     1196        ci2ds rhs, ft0
     1197        ci2ds lhs, ft1
    11981198        divd ft0, ft1
    1199         bcd2i ft1, right, .notInt
     1199        bcd2i ft1, lhs, .notInt
    12001200        storei int32Tag, TagOffset[cfr, index, 8]
    1201         storei right, PayloadOffset[cfr, index, 8]
     1201        storei lhs, PayloadOffset[cfr, index, 8]
    12021202        jmp .done
    12031203    .notInt:
    … …  
    12051205    .done:
    12061206    end,
    1207     macro (left, right) divd left, right end)
     1207    macro (left, right, result) divd left, right, result end)
    12081208
    12091209
    … …  
    12271227        bineq t3, Int32Tag, .slow
    12281228        bineq t2, Int32Tag, .slow
    1229         operation(t1, t0)
     1229        operation(t0, t1)
    12301230        return (t3, t0)
    12311231
    … …  
    12461246
    12471247bitOpProfiled(lshift, OpLshift,
    1248     macro (left, right) lshifti left, right end)
     1248    macro (lhs, rhs) lshifti rhs, lhs end)
    12491249
    12501250
    12511251bitOp(rshift, OpRshift,
    1252     macro (left, right) rshifti left, right end)
     1252    macro (lhs, rhs) rshifti rhs, lhs end)
    12531253
    12541254
    12551255bitOp(urshift, OpUrshift,
    1256     macro (left, right) urshifti left, right end)
     1256    macro (lhs, rhs) urshifti rhs, lhs end)
    12571257
    12581258bitOpProfiled(bitxor, OpBitxor,
    1259     macro (left, right) xori left, right end)
     1259    macro (lhs, rhs) xori rhs, lhs end)
    12601260
    12611261bitOpProfiled(bitand, OpBitand,
    1262     macro (left, right) andi left, right end)
     1262    macro (lhs, rhs) andi rhs, lhs end)
    12631263
    12641264bitOpProfiled(bitor, OpBitor,
    1265     macro (left, right) ori left, right end)
     1265    macro (lhs, rhs) ori rhs, lhs end)
    12661266
    12671267llintOpWithProfile(op_bitnot, OpBitnot, macro (size, get, dispatch, return)

  • trunk/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm

    r284923 r285152  
    11791179        bqb t1, numberTag, .op2NotInt
    11801180        get(m_dst, t2)
    1181         integerOperationAndStore(t1, t0, .slow, t2)
     1181        integerOperationAndStore(t0, t1, .slow, t2)
    11821182
    11831183        updateBinaryArithProfile(size, opcodeStruct, ArithProfileIntInt, t5, t2)
    … …  
    12001200        addq numberTag, t0
    12011201        fq2d t0, ft0
    1202         doubleOperation(ft1, ft0)
     1202        doubleOperation(ft0, ft1, ft0)
    12031203        fd2q ft0, t0
    12041204        subq numberTag, t0
    … …  
    12141214        addq numberTag, t1
    12151215        fq2d t1, ft1
    1216         doubleOperation(ft1, ft0)
     1216        doubleOperation(ft0, ft1, ft0)
    12171217        fd2q ft0, t0
    12181218        subq numberTag, t0
    … …  
    12281228if X86_64 or X86_64_WIN
    12291229    binaryOpCustomStore(div, OpDiv,
    1230         macro (left, right, slow, index)
     1230        macro (lhs, rhs, slow, index)
    12311231            # Assume t3 is scratchable.
    1232             btiz left, slow
    1233             bineq left, -1, .notNeg2TwoThe31DivByNeg1
    1234             bieq right, -2147483648, .slow
     1232            btiz rhs, slow
     1233            bineq rhs, -1, .notNeg2TwoThe31DivByNeg1
     1234            bieq lhs, -2147483648, .slow
    12351235        .notNeg2TwoThe31DivByNeg1:
    1236             btinz right, .intOK
    1237             bilt left, 0, slow
     1236            btinz lhs, .intOK
     1237            bilt rhs, 0, slow
    12381238        .intOK:
    1239             move left, t3
    1240             move right, t0
     1239            move rhs, t3
     1240            move lhs, t0
    12411241            cdqi
    12421242            idivi t3
    … …  
    12451245            storeq t0, [cfr, index, 8]
    12461246        end,
    1247         macro (left, right) divd left, right end)
     1247        macro (left, right, result) divd left, right, result end)
    12481248else
    12491249    slowPathOp(div)
    … …  
    12521252
    12531253binaryOpCustomStore(mul, OpMul,
    1254     macro (left, right, slow, index)
     1254    macro (lhs, rhs, slow, index)
    12551255        # Assume t3 is scratchable.
    1256         move right, t3
    1257         bmulio left, t3, slow
     1256        move lhs, t3
     1257        bmulio rhs, t3, slow
    12581258        btinz t3, .done
    1259         bilt left, 0, slow
    1260         bilt right, 0, slow
     1259        bilt rhs, 0, slow
     1260        bilt lhs, 0, slow
    12611261    .done:
    12621262        orq numberTag, t3
    12631263        storeq t3, [cfr, index, 8]
    12641264    end,
    1265     macro (left, right) muld left, right end)
     1265    macro (left, right, result) muld left, right, result end)
    12661266
    12671267
    12681268macro binaryOp(opcodeName, opcodeStruct, integerOperation, doubleOperation)
    12691269    binaryOpCustomStore(opcodeName, opcodeStruct,
    1270         macro (left, right, slow, index)
    1271             integerOperation(left, right, slow)
    1272             orq numberTag, right
    1273             storeq right, [cfr, index, 8]
     1270        macro (lhs, rhs, slow, index)
     1271            integerOperation(lhs, rhs, slow)
     1272            orq numberTag, lhs
     1273            storeq lhs, [cfr, index, 8]
    12741274        end,
    12751275        doubleOperation)
    … …  
    12771277
    12781278binaryOp(add, OpAdd,
    1279     macro (left, right, slow) baddio left, right, slow end,
    1280     macro (left, right) addd left, right end)
     1279    macro (lhs, rhs, slow) baddio rhs, lhs, slow end,
     1280    macro (left, right, result) addd left, right, result end)
    12811281
    12821282
    12831283binaryOp(sub, OpSub,
    1284     macro (left, right, slow) bsubio left, right, slow end,
    1285     macro (left, right) subd left, right end)
     1284    macro (lhs, rhs, slow) bsubio rhs, lhs, slow end,
     1285    macro (left, right, result) subd left, right, result end)
    12861286
    12871287
    … …  
    13051305        bqb t0, numberTag, .slow
    13061306        bqb t1, numberTag, .slow
    1307         operation(t1, t0)
     1307        operation(t0, t1)
    13081308        orq numberTag, t0
    13091309        return(t0)
    … …  
    13241324
    13251325bitOpProfiled(lshift, OpLshift,
    1326     macro (left, right) lshifti left, right end)
     1326    macro (lhs, rhs) lshifti rhs, lhs end)
    13271327
    13281328
    13291329bitOpProfiled(rshift, OpRshift,
    1330     macro (left, right) rshifti left, right end)
     1330    macro (lhs, rhs) rshifti rhs, lhs end)
    13311331
    13321332
    13331333bitOp(urshift, OpUrshift,
    1334     macro (left, right) urshifti left, right end)
     1334    macro (lhs, rhs) urshifti rhs, lhs end)
    13351335
    13361336bitOpProfiled(bitand, OpBitand,
    1337     macro (left, right) andi left, right end)
     1337    macro (lhs, rhs) andi rhs, lhs end)
    13381338
    13391339bitOpProfiled(bitor, OpBitor,
    1340     macro (left, right) ori left, right end)
     1340    macro (lhs, rhs) ori rhs, lhs end)
    13411341
    13421342bitOpProfiled(bitxor, OpBitxor,
    1343     macro (left, right) xori left, right end)
     1343    macro (lhs, rhs) xori rhs, lhs end)
    13441344
    13451345llintOpWithProfile(op_bitnot, OpBitnot, macro (size, get, dispatch, return)
Note: See TracChangeset for help on using the changeset viewer.