[PATCH] D31287: [mips] Fix atomic compare and swap at O0, v3

[Stefan Maksimovic via Phabricator via llvm-commits]

smaksimovic added a comment.

Mostly comment fix suggestions



================
Comment at: lib/Target/Mips/MipsExpandPseudo.cpp:218-219
+      SC = STI->hasMips32r6() ? Mips::SC_MMR6 : Mips::SC_MM;
+      BNE = STI->hasMIps32r6() ? Mips::BNEC_MMR6 : Mips::BNE_MM;
+      BEQ = STI->hasMIps32r6() ? Mips::BEQC_MMR6 : Mips::BEQ_MM;
+    } else {
----------------
Lowercase i here: `hasMIps32r6()`


================
Comment at: lib/Target/Mips/MipsExpandPseudo.cpp:487
+
+  unsigned LL, SC, ZERO, BNE, BEQ, MOVE;
+
----------------
`BNE` and `MOVE` seem to be unused in any of the `BuildMI` calls in this function.


================
Comment at: lib/Target/Mips/MipsExpandPseudo.cpp:491-494
+      LL = Subtarget.hasMips32r6() ? Mips::LL_MMR6 : Mips::LL_MM;
+      SC = Subtarget.hasMips32r6() ? Mips::SC_MMR6 : Mips::SC_MM;
+      BNE = Subtarget.hasMIps32r6() ? Mips::BNEC_MMR6 : Mips::BNE_MM;
+      BEQ = Subtarget.hasMIps32r6() ? Mips::BEQC_MMR6 : Mips::BEQ_MM;
----------------
`STI->hasMips32r6()`


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1483-1484
+
+  // The scratch registers here with the EarlyClobber | Define | Dead flags
+  // | Implicit is used to persuade the register allocator and the machine
+  // verifier to accept the usage of this register. This has to be a real
----------------
Put `| Implicit` before `flags`,  `registers -> register`


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1487
+  // register which has an UNDEF value but is dead after the instruction which
+  // is unique amoung the registers chosen for the instruction.
+
----------------
`amoung -> among`


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1491
+  // attached to is clobbered before the rest of the inputs are read. Hence it
+  // must be unique amoung the operands to the instruction.
+  // The Define flag is needed to coerce the machine verifier that an Undef
----------------
`amoung -> among`


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1527-1528
+      .addReg(IncrCopy)
+      .addReg(Scratch, RegState::Define |
+                       RegState::EarlyClobber | RegState::Implicit);
 
----------------
In accordance with the comment above, shouldn't the scratch register have the accompanying dead flag as well?


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1691
+  // emitAtomicBinary. In summary, we need a scratch register which is going to
+  // be undef, that is unique amoung the register chosen for the instruction.
 
----------------
Change to `unique among registers`


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1700-1704
+                           RegState::Dead | RegState::Implicit)
+      .addReg(Scratch2, RegState::EarlyClobber | RegState::Define |
+                            RegState::Dead | RegState::Implicit)
+      .addReg(Scratch3, RegState::EarlyClobber | RegState::Define |
+                            RegState::Dead | RegState::Implicit);
----------------
Indentation


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1760
+  // emitAtomicBinary. In summary, we need a scratch register which is going to
+  // be undef, that is unique amoung the register chosen for the instruction.
+
----------------
Change to `unique among registers`


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1768
+      .addReg(Scratch, RegState::EarlyClobber | RegState::Define |
+                           RegState::Dead | RegState::Implicit);
 
----------------
Indentation


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1809
+
+  // The scratch registers here with the EarlyClobber | Define | Dead flags is
+  // used to coerce the register allocator and the machine verifier to accept
----------------
`is -> are`, add Implicit to the list of flags too since it is used in the `addReg()` for `Scratch` and `Scratch2` below.


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1814
+  // attached to is clobbered before the rest of the inputs are read. Hence it
+  // must be unique amoung the operands to the instruction.
+  // The Define flag is needed to coerce the machine verifier that an Undef
----------------
`amoung -> among`


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1877
 
-  //  loop1MBB:
-  //    ll      oldval,0(alginedaddr)
-  //    and     maskedoldval0,oldval,mask
-  //    bne     maskedoldval0,shiftedcmpval,sinkMBB
-  BB = loop1MBB;
-  BuildMI(BB, DL, TII->get(LL), OldVal).addReg(AlignedAddr).addImm(0);
-  BuildMI(BB, DL, TII->get(Mips::AND), MaskedOldVal0)
-    .addReg(OldVal).addReg(Mask);
-  BuildMI(BB, DL, TII->get(Mips::BNE))
-    .addReg(MaskedOldVal0).addReg(ShiftedCmpVal).addMBB(sinkMBB);
-
-  //  loop2MBB:
-  //    and     maskedoldval1,oldval,mask2
-  //    or      storeval,maskedoldval1,shiftednewval
-  //    sc      success,storeval,0(alignedaddr)
-  //    beq     success,$0,loop1MBB
-  BB = loop2MBB;
-  BuildMI(BB, DL, TII->get(Mips::AND), MaskedOldVal1)
-    .addReg(OldVal).addReg(Mask2);
-  BuildMI(BB, DL, TII->get(Mips::OR), StoreVal)
-    .addReg(MaskedOldVal1).addReg(ShiftedNewVal);
-  BuildMI(BB, DL, TII->get(SC), Success)
-      .addReg(StoreVal).addReg(AlignedAddr).addImm(0);
-  BuildMI(BB, DL, TII->get(Mips::BEQ))
-      .addReg(Success).addReg(Mips::ZERO).addMBB(loop1MBB);
-
-  //  sinkMBB:
-  //    srl     srlres,maskedoldval0,shiftamt
-  //    sign_extend dest,srlres
-  BB = sinkMBB;
-
-  BuildMI(BB, DL, TII->get(Mips::SRLV), SrlRes)
-      .addReg(MaskedOldVal0).addReg(ShiftAmt);
-  BB = emitSignExtendToI32InReg(MI, BB, Size, Dest, SrlRes);
+  // The purposes of the flags on the scratch registers is explained in
+  // emitAtomicBinary. In summary, we need a scratch register which is going to
----------------
Either `purposes -> purpose` or `is -> are`


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1879
+  // emitAtomicBinary. In summary, we need a scratch register which is going to
+  // be undef, that is unique amoung the register chosen for the instruction.
+
----------------
Change to `unique among registers`


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1890-1893
+                           RegState::Dead | RegState::Implicit |
+                           RegState::EarlyClobber)
+      .addReg(Scratch2, RegState::EarlyClobber | RegState::Define |
+                            RegState::Dead | RegState::Implicit);
----------------
Indentation, `RegState::EarlyClobber` appears twice for `Scratch`.


================
Comment at: test/CodeGen/Mips/atomic.ll:25
+
+; We want to verify the produced code is well formed all optimization levels, the rest of the tets which ensure correctness.
+; RUN: llc -mtriple=mipsel-unknown-linux-gnu -O1 --disable-machine-licm -mcpu=mips32 -relocation-model=pic -verify-machineinstrs < %s | FileCheck %s --check-prefix=O1
----------------
`tets -> tests`


Repository:
  rL LLVM

https://reviews.llvm.org/D31287