Page MenuHomePhabricator

Get rid of bool parameters in SelectionDAG::getLoad, getStore, and friends.
ClosedPublic

Authored by jlebar on Jul 11 2016, 5:53 PM.

Details

Summary

Instead, we take a single flags arg (a bitset).

Also add a default 0 alignment, and change the order of arguments so the
alignment comes before the flags.

This greatly simplifies many callsites, and fixes a bug in
AMDGPUISelLowering, wherein the order of the args to getLoad was
inverted. It also greatly simplifies the process of adding another flag
to getLoad.

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 63615.Jul 11 2016, 5:53 PM
jlebar retitled this revision from to Get rid of bool parameters in SelectionDag::getLoad, getStore, and friends..
jlebar updated this object.
jlebar added a reviewer: chandlerc.
jlebar added a subscriber: llvm-commits.
jlebar retitled this revision from Get rid of bool parameters in SelectionDag::getLoad, getStore, and friends. to Get rid of bool parameters in SelectionDAG::getLoad, getStore, and friends..Jul 11 2016, 5:54 PM
jlebar edited edge metadata.
chandlerc added inline comments.Jul 11 2016, 5:59 PM
include/llvm/CodeGen/SelectionDAG.h
915–920 ↗(On Diff #63615)

Can you make this an actual enum type instead of unsigned?

jlebar added inline comments.Jul 11 2016, 6:02 PM
include/llvm/CodeGen/SelectionDAG.h
915–920 ↗(On Diff #63615)

That would be really nice, but I don't know of a way to do it safely. It's UB to use (create?) an enum value that's not one of the enumerated values, right? So static_cast<MyEnum>(MyEnum::Foo | MyEnum::Bar) is sadface.

chandlerc edited edge metadata.Jul 11 2016, 6:14 PM

Looked at all the target-independent stuff so far.

include/llvm/CodeGen/SelectionDAG.h
915–920 ↗(On Diff #63615)

To relay the IRC conversation -- this is actually OK because the enumerator has enough bits.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14129–14131 ↗(On Diff #63615)

I feel like this would be more clear as:

unsigned MMOFlags = LLD->getMemOperand()->getFlags();
if (!RLD->isInvariant())
  MMOFlags &= ~MachineMemOperand::MOInvariant;

Or something like that.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4381 ↗(On Diff #63615)

An enum for zero would be nice here.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3536–3538 ↗(On Diff #63615)

Yuck. Better way to do this maybe?

3658–3662 ↗(On Diff #63615)

I think separate statements with |= would be easier to read here than the big expression.

jlebar added inline comments.Jul 11 2016, 6:31 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3658–3662 ↗(On Diff #63615)

Does your evaluation change if I told you that we need a static_cast around every bitwise operation on our enum?

OK, i've looked at all the diffs now.

They all look freaking amazing. Waiting to see the results of enum-ing it, and one nit on one of the targets below.

lib/Target/X86/X86ISelLowering.cpp
5614–5616 ↗(On Diff #63615)

Wait, why on earth do we just ignore the original load's volatility??!?!?!?!!!!????

Ok, this was added in r265013 which was D18546. This filtered volatile loads from getting into this code in the first place.

Can you change this to instead of clearing the volatile bit to assert that the volatile bit is not set coming into this routine?

Seeing this, you might be interested in the enum/bitset library we use in LibreOffice:

https://cgit.freedesktop.org/libreoffice/core/tree/include/o3tl/typed_flags_set.hxx

for stuff like this

Seeing this, you might be interested in the enum/bitset library we use in LibreOffice:

https://cgit.freedesktop.org/libreoffice/core/tree/include/o3tl/typed_flags_set.hxx

for stuff like this

That looks pretty similar to what we had in mind. One part I can't seem to figure out is, why do we have the Wrap class at all? Is it just so you can avoid some of the assertions?

sberg added a subscriber: sberg.Jul 12 2016, 9:40 AM

Seeing this, you might be interested in the enum/bitset library we use in LibreOffice:

https://cgit.freedesktop.org/libreoffice/core/tree/include/o3tl/typed_flags_set.hxx

for stuff like this

That looks pretty similar to what we had in mind. One part I can't seem to figure out is, why do we have the Wrap class at all? Is it just so you can avoid some of the assertions?

that Wrap class is there so that all the operator overloads are only actually available (via SFINAE) for enums that opt into this via an explicit specialization of template<> struct o3tl::typed_flags<TheE>: o3tl::is_typed_flags<TheE, TheM> {};

jlebar updated this revision to Diff 63736.Jul 12 2016, 3:18 PM
jlebar marked 6 inline comments as done.
jlebar edited edge metadata.

Address chandlerc's review.

Thanks for working on this, this is a big improvement. The AMDGPU changes LGTM.

Agreed. This is amazing. Thanks.

chandlerc accepted this revision.Jul 14 2016, 2:04 AM
chandlerc edited edge metadata.

LGTM when the required dependencies finish and land. As others have said, thanks for the cleanup here!

lib/Target/X86/X86ISelLowering.cpp
5606 ↗(On Diff #63736)

Nit: add a message to this assert along the lines of "Cannot merge volatile loads!"

This revision is now accepted and ready to land.Jul 14 2016, 2:04 AM
jlebar updated this revision to Diff 64003.Jul 14 2016, 10:18 AM
jlebar edited edge metadata.

Add message to assert.

This revision was automatically updated to reflect the committed changes.