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

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

One comment left unresolved, re sign-extend -- patch LG other than that!



================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:888
+                        PMV.ShiftAmt, "ValOperand_Shifted");
+  Value *OldResult = TLI->emitMaskedAtomicRMWIntrinsic(
+      Builder, AI, PMV.AlignedAddr, ValOperand_Shifted, PMV.Mask,
----------------
asb wrote:
> jyknight wrote:
> > 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.)
> > 
> Yes I recognise this, and D48129 implements the three optimisations you suggest for RISC-V. I've now reworked that patch so it is target-independent and the same i8/i16 -> i32 transformation logic is used regardless of the AtomicExpansionKind. Let me know what you think,
> 
> It probably makes sense to rebase this patch so it is dependent on D48129.
Sorry, I totally forgot about that other patch! Yes, it makes more sense to structure this way around. :)


================
Comment at: lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:120
+
+static unsigned getLRForRMW32(AtomicOrdering Ordering) {
+  switch (Ordering) {
----------------
asb wrote:
> 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.
Nah, it's fine, not a big deal.


https://reviews.llvm.org/D47882