[PATCH] D97127: [AVR] Improve 8/16 bit atomic operations

[Ayke via Phabricator via llvm-commits]

aykevl created this revision.
aykevl added reviewers: dylanmckay, benshi001.
Herald added subscribers: Jim, jfb, hiraditya.
aykevl requested review of this revision.
Herald added a project: LLVM.

There were some serious issues with atomic operations. This patch should
fix the biggest issues.

For details on the issue take a look at this Compiler Explorer sample:
https://godbolt.org/z/n3ndhn

Code:

  void atomicadd(_Atomic char *val) {
      *val += 5;
  }

Output:

  atomicadd:
      movw    r26, r24
      ldi     r24, 5     ; 'operand' register
      in      r0, 63
      cli
      ld      r24, X     ; load value
      add     r24, r26   ; value += X
      st      X, r24     ; store value back
      out     63, r0
      ret                ; return the wrong value (in r24)

There are various problems with this.

- The value to add (5) is stored in r24. However, the value to add to is loaded in the same register: r24.
- The `add` instruction adds half of the pointer to the loaded value, instead of (attempting to) add the operand with value 5.
- The output value of the cmpxchg instruction (which is not used in this code sample) is the new value with 5 added, not the old value. The LangRef specifies that it has to be the old value, before the operation.

This patch fixes the first two and leaves the third problem to be fixed
at a later date. I believe atomics were mostly broken before this patch,
with this patch they should become usable as long as you ignore the
output of the atomic operation. In particular it fixes the following
things:

- It sets the earlyclobber flag for the input ('$operand' operand) so that the register allocator puts it in a different register than the output value.
- It fixes a number of issues with the pseudo op expansion pass, for example now it adds the $operand field instead of the pointer. This fixes most machine instruction verifier issues (other flagged issues are unrelated to atomics).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97127

Files:
  llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
  llvm/lib/Target/AVR/AVRInstrInfo.td


Index: llvm/lib/Target/AVR/AVRInstrInfo.td
===================================================================
--- llvm/lib/Target/AVR/AVRInstrInfo.td
+++ llvm/lib/Target/AVR/AVRInstrInfo.td
@@ -1299,6 +1299,7 @@
   Pseudo<(outs), (ins PTRRC:$rd, DRC:$rr), "atomic_op",
          [(Op i16:$rd, DRC:$rr)]>;
 
+let Constraints = "@earlyclobber $rd" in
 class AtomicLoadOp<PatFrag Op, RegisterClass DRC,
                    RegisterClass PTRRC> :
   Pseudo<(outs DRC:$rd), (ins PTRRC:$rr, DRC:$operand),
Index: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
===================================================================
--- llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
+++ llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
@@ -883,20 +883,24 @@
                                                Block &MBB,
                                                BlockIt MBBI) {
   return expandAtomic(MBB, MBBI, [&](MachineInstr &MI) {
-      auto Op1 = MI.getOperand(0);
-      auto Op2 = MI.getOperand(1);
+      auto DstReg = MI.getOperand(0).getReg();
+      auto PtrOp = MI.getOperand(1);
+      auto SrcReg = MI.getOperand(2).getReg();
 
       unsigned LoadOpcode = (Width == 8) ? AVR::LDRdPtr : AVR::LDWRdPtr;
       unsigned StoreOpcode = (Width == 8) ? AVR::STPtrRr : AVR::STWPtrRr;
 
+      // FIXME: this returns the new value (after the operation), not the old
+      // value as the atomicrmw instruction is supposed to do!
+
       // Create the load
-      buildMI(MBB, MBBI, LoadOpcode).add(Op1).add(Op2);
+      buildMI(MBB, MBBI, LoadOpcode, DstReg).addReg(PtrOp.getReg());
 
       // Create the arithmetic op
-      buildMI(MBB, MBBI, ArithOpcode).add(Op1).add(Op1).add(Op2);
+      buildMI(MBB, MBBI, ArithOpcode, DstReg).addReg(DstReg).addReg(SrcReg);
 
       // Create the store
-      buildMI(MBB, MBBI, StoreOpcode).add(Op2).add(Op1);
+      buildMI(MBB, MBBI, StoreOpcode).add(PtrOp).addReg(DstReg);
   });
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D97127.325245.patch
Type: text/x-patch
Size: 1927 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210220/edb90c5f/attachment.bin>