Page MenuHomePhabricator

Add AtomicExpandPass::bracketInstWithFences, and use it whenever getInsertFencesForAtomic would trigger in SelectionDAGBuilder
ClosedPublic

Authored by morisset on Sep 3 2014, 4:10 PM.

Details

Summary

The goal is to eventually remove all the code related to getInsertFencesForAtomic
in SelectionDAGBuilder as it is wrong (designed for ARM, not really portable, works
mostly by accident because the backends are overly conservative), and repeats the
same logic that goes in emitLeading/TrailingFence.

In this patch, I make AtomicExpandPass insert the fences as it knows better
where to put them. Because this requires getting the fences and not just
passing an IRBuilder around, I had to change the return type of
emitLeading/TrailingFence.
This code only triggers on ARM for now. Because it is earlier in the pipeline
than SelectionDAGBuilder, it triggers and lowers atomic accesses to atomic so
SelectionDAGBuilder does not add barriers anymore on ARM.

If this patch is accepted I plan to implement emitLeading/TrailingFence for all
backends that setInsertFencesForAtomic(true), which will allow both making them
less conservative and simplifying SelectionDAGBuilder once they are all using
this interface.

Depends on D5090 (very minor dependency, both just happen to modify
AtomicExpandPass::runOnFunction).

Diff Detail

Repository
rL LLVM

Event Timeline

morisset updated this revision to Diff 13232.Sep 3 2014, 4:10 PM
morisset retitled this revision from to Add AtomicExpandPass::bracketInstWithFences, and use it whenever getInsertFencesForAtomic would trigger in SelectionDAGBuilder.
morisset updated this object.
morisset edited the test plan for this revision. (Show Details)
morisset added reviewers: jfb, t.p.northover.
morisset added a subscriber: Unknown Object (MLST).
morisset updated this revision to Diff 13846.Sep 18 2014, 1:02 PM

Cleanup of some now redundant code that I had missed.

I would love getting some reviews on this patch, other patches depend on it
(D5180 being one that is otherwise ready to land). Thanks !

jfb added inline comments.Sep 22 2014, 9:02 AM
include/llvm/Target/TargetLowering.h
963 ↗(On Diff #13846)

What should the return value be? Are there cases where a backend emits more than one instruction?

lib/CodeGen/AtomicExpandPass.cpp
167 ↗(On Diff #13846)

Should this still be here, since the code wants to handle more than just ARM?

Thanks for the review, I will fix the comments as described below.

include/llvm/Target/TargetLowering.h
963 ↗(On Diff #13846)

The return value is either nullptr (if the backend did not insert anything) or a pointer to the inserted (IR-level) instruction. There is no reason for a backend to insert several (IR-level) instructions, the only thing that makes sense for a backend to insert is an intrinsic corresponding to its hardware fence(s). Even if it requires several machine instructions for a fence, it can still be just one IR-level instruction.
I will add a comment to that effect.

lib/CodeGen/AtomicExpandPass.cpp
167 ↗(On Diff #13846)

Probably not exactly as worded. I will reword it by saying that on some architectures load-linked is atomic for bigger sizes, and that for example on ARM...

jfb edited edge metadata.Sep 22 2014, 11:04 AM

Probably worth putting in the commit message: no tests changed because this shouldn't cause any functional changes?

morisset updated this revision to Diff 13998.Sep 23 2014, 9:36 AM
morisset edited edge metadata.

Updated comments.

jfb added inline comments.Sep 23 2014, 10:32 AM
lib/CodeGen/AtomicExpandPass.cpp
168 ↗(On Diff #13998)

That reminds me: ARM guarantees that ldrd and strd are atomic if the CPU has LPAE (e.g. A15 has that guarantee, see DDI0406C ARM architecture reference manual, sections A8.8.72-74 LDRD). Could you add this to your list of things to implement (emit ldrd/strd when appropriate)?

lib/Target/ARM/ARMISelLowering.cpp
11052 ↗(On Diff #13998)

Fold this into the default case for the switch.

11073 ↗(On Diff #13998)

Same.

rnk added a subscriber: rnk.Sep 23 2014, 11:24 AM
rnk added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
11052 ↗(On Diff #13998)

We can't do this because MSVC will warn about falling off the end of the function.

morisset added inline comments.Sep 23 2014, 11:44 AM
lib/CodeGen/AtomicExpandPass.cpp
168 ↗(On Diff #13998)

I just commited a FIXME to that effect in r218326.

jfb accepted this revision.Sep 23 2014, 1:30 PM
jfb edited edge metadata.

Looks good.

lib/CodeGen/AtomicExpandPass.cpp
168 ↗(On Diff #13998)

Thanks!

lib/Target/ARM/ARMISelLowering.cpp
11052 ↗(On Diff #13998)

Grumble grumble MSVC.

This revision is now accepted and ready to land.Sep 23 2014, 1:30 PM
morisset closed this revision.Sep 23 2014, 1:41 PM
morisset updated this revision to Diff 14015.

Closed by commit rL218329 (authored by @morisset).