Page MenuHomePhabricator

[AVR] Fix atomicrmw result value
ClosedPublic

Authored by aykevl on Jan 19 2022, 2:44 PM.

Details

Summary

This patch fixes the atomicrmw result value to be the value before the operation instead of the value after the operation. This was a bug, left as a FIXME in the code (see D97127).

From the LangRef:

The contents of memory at the location specified by the ‘<pointer>’ operand are atomically read, modified, and written back. The original value at the location is returned.

Doing this expansion early allows the register allocator to arrange registers in such a way that commutable operations are simply swapped around as needed, which results in shorter code while still being correct.

Diff Detail

Event Timeline

aykevl created this revision.Jan 19 2022, 2:44 PM
aykevl requested review of this revision.Jan 19 2022, 2:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2022, 2:44 PM
benshi001 added inline comments.Jan 21 2022, 6:08 AM
llvm/lib/Target/AVR/AVRISelLowering.cpp
1705

Also, do we need to make a helper function int getRegScratch(void) in AVRSubtarget.h, just like I have done as getIORegRAMPZ(void) ? The new helper function will return r16 for avr-tiny devices.

Also, do we need a helper function int getIORegSREG(void) instead of hard coded 0x3f ?

benshi001 added inline comments.Jan 21 2022, 6:13 AM
llvm/lib/Target/AVR/AVRISelLowering.cpp
1733

Do we need a flag RegState::Define for Result ?

aykevl updated this revision to Diff 402240.Jan 22 2022, 11:02 AM
  • use getIORegSREG() instead of hardcoding I/O address 0x3f.
aykevl added inline comments.Jan 22 2022, 11:05 AM
llvm/lib/Target/AVR/AVRISelLowering.cpp
1705

See my comment in D117426.

1733

No, because BuildMI will always set RegState::Define for the last parameter (Result in this case).

aykevl added inline comments.Jan 22 2022, 11:07 AM
llvm/lib/Target/AVR/AVRISelLowering.cpp
1705

Sorry I mean D117425.

benshi001 added inline comments.Jan 23 2022, 8:16 AM
llvm/test/CodeGen/AVR/atomics/load16.ll
32

I am OK with this patch. Just a bit confusion.

In the next test file load8.ll, RR1 is the address and RD is the loaded value.

ld [[RD:r[0-9]+]], [[RR:(X|Y|Z)]]

I suggest you make them in accordance with each other when committing.

benshi001 accepted this revision.Jan 23 2022, 8:16 AM
This revision is now accepted and ready to land.Jan 23 2022, 8:16 AM
aykevl updated this revision to Diff 402343.Jan 23 2022, 9:00 AM
  • small update to the load8.ll test to make it more readable
aykevl added inline comments.Jan 23 2022, 9:05 AM
llvm/test/CodeGen/AVR/atomics/load16.ll
32

Sounds reasonable. However, I would prefer to do this in a separate patch so that the diff in this patch is smaller. Here it is: D117991
If you accept that patch I can commit them after each other.

benshi001 added inline comments.Jan 23 2022, 7:34 PM
llvm/test/CodeGen/AVR/atomics/load16.ll
32

OK. Please commit them. :)

This revision was landed with ongoing or failed builds.Feb 2 2022, 12:10 AM
This revision was automatically updated to reflect the committed changes.