Page MenuHomePhabricator

[AVR] Improve 8/16 bit atomic operations
Needs ReviewPublic

Authored by aykevl on Feb 20 2021, 2:25 PM.

Details

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.

Diff Detail

Event Timeline

aykevl created this revision.Feb 20 2021, 2:25 PM
aykevl requested review of this revision.Feb 20 2021, 2:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2021, 2:25 PM
aykevl edited the summary of this revision. (Show Details)Feb 20 2021, 2:29 PM
benshi001 added a comment.EditedFeb 28 2021, 7:20 PM

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 added a comment.Mar 3 2021, 6:27 AM

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 added a comment.EditedMar 3 2021, 6:42 PM

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 added a comment.Mar 4 2021, 5:13 AM

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?

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.