From 431a9414655ba4825a46e6765ef50a0b4ef7e101 Mon Sep 17 00:00:00 2001 From: Ayke van Laethem Date: Sat, 20 Feb 2021 20:51:38 +0100 Subject: [PATCH] [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 --- llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp | 14 +++++++++----- llvm/lib/Target/AVR/AVRInstrInfo.td | 1 + 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp b/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp index 69cb76d..92d5e92 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 fa1246e..2831e2e 100644 --- a/llvm/lib/Target/AVR/AVRInstrInfo.td +++ b/llvm/lib/Target/AVR/AVRInstrInfo.td @@ -1297,6 +1297,7 @@ class AtomicStore; +let Constraints = "@earlyclobber $rd" in class AtomicLoadOp : Pseudo<(outs DRC:$rd), (ins PTRRC:$rr, DRC:$operand), -- 2.7.4