Page MenuHomePhabricator

[Atomic] Remove IsStore/IsLoad in the interface, and pass the instruction instead. NFC.
ClosedPublic

Authored by timshen on May 2 2017, 2:56 PM.

Details

Summary

Now both emitLeadingFence and emitTrailingFence take the instruction itself, instead of taking IsLoad/IsStore pairs. Instruction::mayReadFromMemory and Instrucion::mayWriteToMemory are used for determining those two booleans.

The instruction argument is also useful for later D32763, in emitTrailingFence. For emitLeadingFence, it seems to have cleaner interface with the proposed change.

Diff Detail

Repository
rL LLVM

Event Timeline

timshen created this revision.May 2 2017, 2:56 PM
timshen retitled this revision from [PPC] Let emitTrailingFence take the loaded value. NFC. to [Atomics] Let emitTrailingFence take the loaded value. NFC..
kbarton edited edge metadata.May 4 2017, 7:43 PM

D32763 only handles the trailing fences. Is there going to be another patch that deals with the leading fences as well?

t.p.northover edited edge metadata.May 5 2017, 7:07 AM

There is no value at the leading fence, it's specifically the fence emitted before the load.

D32763 only handles the trailing fences. Is there going to be another patch that deals with the leading fences as well?

No, because the reason trailing fence can depend on the loaded value is that, trailing fence is after the load in program order. IIRC data/control dependencies follows the program order.

jyknight edited edge metadata.May 5 2017, 9:31 AM
D32763 only handles the trailing fences. Is there going to be another patch that deals with the leading fences as well?

No, because the reason trailing fence can depend on the loaded value is that, trailing fence is after the load in program order. IIRC data/control dependencies follows the program order.

Why does this add Instruction* argument to the emitLeadingFence function, then?

llvm/include/llvm/Target/TargetLowering.h
1434 ↗(On Diff #97507)

Didn't modify the docs as to what this Instruction* is, and what it should be used for. Nor does it even have a name.

llvm/lib/CodeGen/AtomicExpandPass.cpp
1098 ↗(On Diff #97507)

It feels a bit strange to pass the two-valued CI result struct here.

timshen updated this revision to Diff 97986.May 5 2017, 10:49 AM

Patch rewritten.

Now both emitLeadingFence and emitTrailingFence take the instruction itself, instead of taking IsLoad/IsStore pairs. Instead, Instruction::mayReadFromMemory and Instrucion::mayWriteToMemory are used for determining those two booleans.

The instruction argument is also useful for later D32763, in emitTrailingFence. For emitLeadingFence, it seems to have cleaner interface with the proposed change.

timshen marked an inline comment as done.May 5 2017, 10:51 AM
timshen added inline comments.
llvm/lib/CodeGen/AtomicExpandPass.cpp
1098 ↗(On Diff #97507)

I'm not sure what is the "two-valued CL result", but I removed the propagation of IsLoad and IsStore, and now the code seems cleaner and more flexible.

timshen retitled this revision from [Atomics] Let emitTrailingFence take the loaded value. NFC. to [Atomic] Remove IsStore/IsLoad. NFC..May 5 2017, 11:03 AM
timshen edited the summary of this revision. (Show Details)
timshen retitled this revision from [Atomic] Remove IsStore/IsLoad. NFC. to [Atomic] Remove IsStore/IsLoad in the interface, and pass the instruction instead. NFC..May 5 2017, 11:44 AM
timshen planned changes to this revision.May 5 2017, 2:01 PM

I got some miscompiles. Investigating.

echristo edited edge metadata.May 5 2017, 2:12 PM

Might want to remove the NFC then :)

timshen updated this revision to Diff 98026.May 5 2017, 3:16 PM

Instruction::mayReadFromMemory and Instruction::mayWriteToMemory has their own subtlties. Created Instruction::isAtomicLoad and Instruction::isAtomicStore instead.

Might want to remove the NFC then :)

Now its' NFC. :)

sanjoy added a subscriber: sanjoy.May 7 2017, 1:22 PM

Random drive by comment.

llvm/include/llvm/IR/Instruction.h
460 ↗(On Diff #98026)

Would it be better to call this hasAtomicLoad? Since the instructions here do not *solely* atomic-load.

This seems basically fine modulo renaming and comments.

llvm/include/llvm/IR/Instruction.h
460 ↗(On Diff #98026)

Agree.

llvm/include/llvm/Target/TargetLowering.h
1399 ↗(On Diff #98026)

Maybe say something like:

Inst is the original atomic instruction, prior to other expansions that may be performed.

timshen updated this revision to Diff 98295.May 9 2017, 8:28 AM

s/isAtomic/hasAtomic/ and update comments.

timshen marked 3 inline comments as done.May 9 2017, 8:29 AM
jyknight accepted this revision.May 9 2017, 8:31 AM
This revision is now accepted and ready to land.May 9 2017, 8:31 AM
This revision was automatically updated to reflect the committed changes.