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

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:120
+
+static unsigned getLRForRMW32(AtomicOrdering Ordering) {
+  switch (Ordering) {
----------------
jyknight wrote:
> 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?
Instruction definitions in the RISC-V backend follow the instruction encoding, meaning they are easy to inspect for correctness with respect to the ISA manual and the MC layer support is trivial. We could define a `RISCV::PseudoLR` that took `AtomicOrdering` and have logic that selected the appropriate AQ/RL bits, but we'd still be implementing something equivalent to this switch - just moving it elsewhere.


================
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);
----------------
jyknight wrote:
> Nice -- the sequence in AtomicExpandPass::performMaskedAtomicOp should be updated to do the same thing, I think!
I've added a TODO to performMaskedAtomicOp


https://reviews.llvm.org/D47882