[AVR] Improve 8/16 bit atomic operations

[aykevl]

Summary

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

The main goal of this patch is to make the AVR backend machine verifier clean, but it should also be an improvement separate from that.
With this patch, I think I've submitted patches for all machine verifier issues that may have a significant impact, most other changes are minor test case fixes (.mir files) and fixes to various lifeness issues.

[benshi001]

Do we need a test case to show the right AVR assembly ? Or at least shows that the points #1 and #2 in your comment are fixed.

[aykevl]

In D97127#2593329, @benshi001 wrote:

Do we need a test case to show the right AVR assembly ? Or at least shows that the points #1 and #2 in your comment are fixed.

I think that would imply using hardcoded registers in the test. Is that what you mean?

[benshi001]

In D97127#2600159, @aykevl wrote:

In D97127#2593329, @benshi001 wrote:

Do we need a test case to show the right AVR assembly ? Or at least shows that the points #1 and #2 in your comment are fixed.

I think that would imply using hardcoded registers in the test. Is that what you mean?

No. I meant, you have shown a case void atomicadd(_Atomic char *val) and corresponding assembly, and I would like to see the change of the generated assembly by your patch, and wonder if the change can be added as a test case.

[aykevl]

In D97127#2602032, @benshi001 wrote:

In D97127#2600159, @aykevl wrote:

In D97127#2593329, @benshi001 wrote:

Do we need a test case to show the right AVR assembly ? Or at least shows that the points #1 and #2 in your comment are fixed.

I think that would imply using hardcoded registers in the test. Is that what you mean?

No. I meant, you have shown a case void atomicadd(_Atomic char *val) and corresponding assembly, and I would like to see the change of the generated assembly by your patch, and wonder if the change can be added as a test case.

There are tests in llvm/test/CodeGen/AVR/atomics/load8.ll for example. However while the assembly has changed, patterns like [[RR:r[0-9]+]] are used so the test does not change. Therefore, I think the only way to show the bug is fixed is by hardcoding registers.
Or do you see another way?

[benshi001]

In D97127#2603227, @aykevl wrote:

In D97127#2602032, @benshi001 wrote:

In D97127#2600159, @aykevl wrote:

In D97127#2593329, @benshi001 wrote:

Do we need a test case to show the right AVR assembly ? Or at least shows that the points #1 and #2 in your comment are fixed.

I think that would imply using hardcoded registers in the test. Is that what you mean?

No. I meant, you have shown a case void atomicadd(_Atomic char *val) and corresponding assembly, and I would like to see the change of the generated assembly by your patch, and wonder if the change can be added as a test case.

There are tests in llvm/test/CodeGen/AVR/atomics/load8.ll for example. However while the assembly has changed, patterns like [[RR:r[0-9]+]] are used so the test does not change. Therefore, I think the only way to show the bug is fixed is by hardcoding registers.
Or do you see another way?

I see. There is no need to do so.

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

bool AVRExpandPseudo::expandAtomicBinaryOp(unsigned Opcode,
   return expandAtomicBinaryOp(Opcode, MBB, MBBI, [](MachineInstr &MI) {});
 }
 
 bool AVRExpandPseudo::expandAtomicArithmeticOp(unsigned Width,
                                                unsigned ArithOpcode,
                                                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);
   });
 }
 
 Register AVRExpandPseudo::scavengeGPR8(MachineInstr &MI) {
   MachineBasicBlock &MBB = *MI.getParent();
   RegScavenger RS;
 
   RS.enterBasicBlock(MBB);

llvm/lib/Target/AVR/AVRInstrInfo.td

   Pseudo<(outs DRC:$rd), (ins PTRRC:$rr), "atomic_op",
          [(set DRC:$rd, (Op i16:$rr))]>;
 
 class AtomicStore<PatFrag Op, RegisterClass DRC,
                   RegisterClass PTRRC> :
   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),
          "atomic_op",
          [(set DRC:$rd, (Op i16:$rr, DRC:$operand))]>;
 
 // FIXME: I think 16-bit atomic binary ops need to mark
 // r0 as clobbered.