This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Avoid emitting hardware fences for singlethread fences
ClosedPublic

Authored by reames on Jan 9 2023, 11:20 AM.

Details

Summary

singlethread fences only synchronize with code running on the same hardware thread (i.e. signal handlers). Because of this, we need to prevent instruction reordering, but do not need to emit hardware fence instructions.

The implementation strategy here matches many other backends. The main motivation of this patch is to introduce the MEMBARRIER node and get some test coverage for it.

Diff Detail

Event Timeline

reames created this revision.Jan 9 2023, 11:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2023, 11:20 AM
reames requested review of this revision.Jan 9 2023, 11:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2023, 11:20 AM
reames edited the summary of this revision. (Show Details)Jan 9 2023, 11:24 AM

If every backend is doing it shouldn't this be a generic thing rather than copy/pasted to every backend?

llvm/lib/Target/RISCV/RISCVInstrInfo.td
1849

This isn't standard naming for pseudos, but maybe other backends do it this way too?...

If every backend is doing it shouldn't this be a generic thing rather than copy/pasted to every backend?

Agreed. I looked briefly at doing this. I think we could probably share the SDAG pieces; the pseudos look less obvious. Happy to take a pass at this, but I'd prefer that be non-blocking for this change. You okay with that?

llvm/lib/Target/RISCV/RISCVInstrInfo.td
1849

I honestly copied this from X86 without trying to understand it closely. If you have suggestions, happy to apply them.

There was an attempt to make a target independent compiler barrier here. https://reviews.llvm.org/D92842 I haven't re-read through the discussion yet.

If every backend is doing it shouldn't this be a generic thing rather than copy/pasted to every backend?

Agreed. I looked briefly at doing this. I think we could probably share the SDAG pieces; the pseudos look less obvious. Happy to take a pass at this, but I'd prefer that be non-blocking for this change. You okay with that?

Yeah, I just wish it were done that way from the start :( busywork as a result

llvm/lib/Target/RISCV/RISCVInstrInfo.td
1849

Normally it'd be riscv_membarrier defined at the top of the file and PseudoMEMBARRIER defined here (or in the instructions section)

reames planned changes to this revision.Jan 9 2023, 12:31 PM

If every backend is doing it shouldn't this be a generic thing rather than copy/pasted to every backend?

Agreed. I looked briefly at doing this. I think we could probably share the SDAG pieces; the pseudos look less obvious. Happy to take a pass at this, but I'd prefer that be non-blocking for this change. You okay with that?

Yeah, I just wish it were done that way from the start :( busywork as a result

The SDAG part turned out to be easier than I'd realized, and is now posted: https://reviews.llvm.org/D141317

I'm going to rebase this patch once that one lands.

asb accepted this revision.Jan 10 2023, 5:32 AM

LGTM - two minor nits inline.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3677

handles => handlers

3687

Accidental extra whitespace?

This revision is now accepted and ready to land.Jan 10 2023, 5:32 AM
This revision was landed with ongoing or failed builds.Jan 10 2023, 10:10 AM
This revision was automatically updated to reflect the committed changes.