[PATCH] D47882: [RISCV] Codegen for i8, i16, and i32 atomicrmw with RV32A

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: include/llvm/CodeGen/TargetLowering.h:1561
+                                              AtomicOrdering Ord) const {
+    llvm_unreachable("Custom atomicrmw expansion unimplemented on this target");
+  }
----------------
s/Custom/Masked/


================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:888
+                        PMV.ShiftAmt, "ValOperand_Shifted");
+  Value *OldResult = TLI->emitMaskedAtomicRMWIntrinsic(
+      Builder, AI, PMV.AlignedAddr, ValOperand_Shifted, PMV.Mask,
----------------
For the OR, XOR, and AND operations, there's no need to implement or use a masked intrinsic at all -- the normal 32bit atomic will do the job fine given ValOperand_Shifted as the argument.

For the masked AND, you do need to pass "ValOperand_Shifted | PMV.Inv_Mask" instead, of course.

(I note that I neglected to implement that optimization for AND in the cmpxchg expansion in expandPartwordAtomicRMW.)



================
Comment at: lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:120
+
+static unsigned getLRForRMW32(AtomicOrdering Ordering) {
+  switch (Ordering) {
----------------
It's not a big deal, but this function and others like it make me think it'd be better to just have one instruction for LR and one for SC, with an argument for the atomic ordering. I assume there's some reason that was not doable?


================
Comment at: lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:204
+  // We select bits from newval and oldval using:
+  // https://graphics.stanford.edu/~seander/bithacks.html#MaskedMerge
+  // r = oldval ^ ((oldval ^ newval) & masktargetdata);
----------------
Nice -- the sequence in AtomicExpandPass::performMaskedAtomicOp should be updated to do the same thing, I think!


================
Comment at: lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:379
+  //   mv scratch1, destreg
+  //   ifnochangeneeded scratch2, incr, .looptail
+  BuildMI(LoopHeadMBB, DL, TII->get(getLRForRMW32(Ordering)), DestReg)
----------------
Doesn't this need to sign-extend the values prior to doing a signed comparison?


https://reviews.llvm.org/D47882