[llvm] 431a941 - [AVR] Improve 8/16 bit atomic operations

[Ayke van Laethem via llvm-commits]

Author: Ayke van Laethem
Date: 2021-07-24T14:03:26+02:00
New Revision: 431a9414655ba4825a46e6765ef50a0b4ef7e101

URL: https://github.com/llvm/llvm-project/commit/431a9414655ba4825a46e6765ef50a0b4ef7e101
DIFF: https://github.com/llvm/llvm-project/commit/431a9414655ba4825a46e6765ef50a0b4ef7e101.diff

LOG: [AVR] Improve 8/16 bit atomic operations

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).

Differential Revision: https://reviews.llvm.org/D97127

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp b/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
index 69cb76dd25eb..92d5e92bb483 100644
--- a/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
+++ b/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
@@ -895,20 +895,24 @@ bool AVRExpandPseudo::expandAtomicArithmeticOp(unsigned Width,
                                                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);
   });
 }
 

diff  --git a/llvm/lib/Target/AVR/AVRInstrInfo.td b/llvm/lib/Target/AVR/AVRInstrInfo.td
index fa1246e16ea3..2831e2e02ca7 100644
--- a/llvm/lib/Target/AVR/AVRInstrInfo.td
+++ b/llvm/lib/Target/AVR/AVRInstrInfo.td
@@ -1297,6 +1297,7 @@ class AtomicStore<PatFrag Op, RegisterClass DRC,
   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),