This is an archive of the discontinued LLVM Phabricator instance.

[X86][GlobalISEL] Support lowering aligned unordered atomics
ClosedPublic

Authored by reames on Feb 5 2019, 7:33 PM.

Details

Summary

This is a companion to D57601, but will be landed afterwards. I don't want to have to worry about two sources of atomic MMOs, some volatile, some not.

The existing lowering code is accidentally correct for unordered atomics as far as I can tell. An unordered atomic has no memory ordering, and simply requires the actual load or store to be done as a single well aligned instruction. As such, relax the restriction while adding tests to ensure the lowering remains correct in the future.

Diff Detail

Repository
rL LLVM

Event Timeline

reames created this revision.Feb 5 2019, 7:33 PM
arsenm added a subscriber: arsenm.Feb 6 2019, 11:20 AM
arsenm added inline comments.
test/CodeGen/X86/atomic-unordered.ll
2–6 ↗(On Diff #185475)

I don't know about mixing GlobalISel and SelectionDAG tests.

I would also expect this to be a MIR pass which just runs the selector, but it looks like x86 is using a number of end-to-end tests for this already

jlebar resigned from this revision.Feb 6 2019, 11:49 AM

This seems reasonable to me but I'm not familiar enough with the x86 backend to feel comfortable signing off on it.

igorb added a comment.Feb 6 2019, 11:52 PM

The change looks ok.
Could you please add mir test for InstructionSelector pass, similar to select-memop-scalar.mir.
Thanks

This revision was not accepted when it landed; it landed in state Needs Review.Mar 15 2019, 10:49 AM
This revision was automatically updated to reflect the committed changes.
reames marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2019, 10:49 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript

The change looks ok.
Could you please add mir test for InstructionSelector pass, similar to select-memop-scalar.mir.
Thanks

Went ahead and landed this with the mir test as requested, but honestly, I think the mir test is much less useful and readable than a plain IR test. The sheer amount of *duplication* in the MIR file was distressing.

What I'd really have preferred - for global ISEL specific tests - was a version which started with IR, but used -stop-after=instruction-selector. I couldn't find a way to do that w/an auto updated test though.