This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Use a union of bitfield structs for SDNode::SubclassData.
ClosedPublic

Authored by jlebar on Aug 1 2016, 2:33 PM.

Diff Detail

Event Timeline

jlebar updated this revision to Diff 66383.Aug 1 2016, 2:33 PM
jlebar retitled this revision from to [SelectionDAG] Use a union of bitfield structs for SDNode::SubclassData..
jlebar updated this object.
jlebar added a reviewer: chandlerc.
jlebar added a subscriber: llvm-commits.
jlebar updated this revision to Diff 66557.Aug 2 2016, 2:26 PM

Update per bitfield API discussion in D23035.

chandlerc added inline comments.Aug 16 2016, 5:48 PM
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
408–411

How much are getters and setters at this layer really providing?

If we need these, I'd expect to want them in the actual SDNode public API. For participants in the SDNode type hierarchy, they don't seem to add a lot of value over directly referencing the bitfield.

413–414

You can't actually do this. An underscore followed by a capital letter is a reserved identifier.

But see above, I don't think we need to do this.

415

using an explicitly *sized* type for a bitfield seems... actively confusing. I'd just use 'unsigned'.

jlebar added inline comments.Aug 16 2016, 5:57 PM
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
408–411

How much are getters and setters at this layer really providing?

I think it was your idea! :)

The notion iirc was that

  • we want wrappers for writing enum type-fields in order to check that we're not writing a too-wide value,
  • we want wrappers for reading strongly-typed enum-type fields so we don't have to static_cast everywhere,
  • we wanted to be consistent with these for the bool fields, and
  • we get some type-safety on the bool fields this way too.

If we need these, I'd expect to want them in the actual SDNode public API

I don't quite see how that would work. We don't want to expose all of these getters in SDNode itself because some are only valid to read if we're e.g. a MemSDNode or whatever.

chandlerc added inline comments.Aug 16 2016, 6:05 PM
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
408–411

I think I was trying to suggest that accessors would make sense on the *public* API...

And in fact, the ones that we need it looks like are already there, so this is probably just already the case today.

I'm fine if we need a few to dodge static_cast goop for enums, but for all the bools I would just directly access them from within the SDNode type hierarchy code.

jlebar updated this revision to Diff 68302.Aug 16 2016, 9:03 PM

Get rid of getters/setters entirely.

FWIW I'm not totally wild about this, because the assertions are verbose. It
brings me back to the earlier idea in D23035 of using a macro to combine the
assignment and assertion.

But I'm happy to leave this how it is too.

jlebar marked 4 inline comments as done.Aug 16 2016, 9:04 PM
jlebar added inline comments.
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
415

They pack differently? https://godbolt.org/g/dwzPfq

We're trying to pack these with an int16_t (SDNode::NodeType).

Friendly ping.

chandlerc edited edge metadata.Aug 22 2016, 11:37 PM

Mostly minor stuff. This is pretty close I think.

llvm/include/llvm/CodeGen/SelectionDAGNodes.h
415

Cool, comment at the top of the list of bitfields that:

a) We define a set of mini helper classes to hold all the bitfields used.
b) The total is designed to pack with an int16_t and so shouldn't exceed 16-bits and should uint16_t as the type.

450–456

Is this just a getter/setter pair you missed or was this specifically intended to deviate from the pattern? (If the latter, a comment would be good.)

1202–1203

Would the form using the accessor have the same effect:

assert(getOrdering() == SuccessOrdering && "Value truncated");
1205

If this compiles, then the above could probably be written this way as well.

I'm fine with whichever way of writing this accomplishes your intent and is shorter.

1818–1819

Where did these asserts go? I may just be missing the reason they're already covered, sorry if so...

1846–1847

Just marking the other place where asserts seem to be lost.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5086–5091

I think you should preface this with an explanation of why you're building a node at all:

// We build a synthetic LoadSDNode to extract its subclass data as
// an integer for the folding set. The compiler can reduce ...

That will help the reader make sense of the subclass data being relevant when they've not yet gotten to the member function call on the temporary LoadSDNode.

Actually, I see that this pattern repeats itself a bunch. Not sure how best to handle it as the repeated comment would indeed become quite tedious.

My only idea would be to keep a helper routine but make it look more like:

template <typename SDNodeT, typename ...Ts>
uint16_t getSyntheticNodeSubclassData(const SDLoc &dl, Ts&&... Args) {
  return SDNodeT(dl.getIROrder(), DebugLoc(), std::forward<Ts>(Args)...)
             .getRawSubclassData();
}

And then you could attach your comment there. But that's a mighty magical helper.

Anyways, I don't think this is a blocking issue if you don't like this idea and can't come up with anything better.

jlebar updated this revision to Diff 69008.Aug 23 2016, 9:50 AM
jlebar marked 7 inline comments as done.
jlebar edited edge metadata.

Second round of chandlerc's review.

Thank you for the review, Chandler.

llvm/include/llvm/CodeGen/SelectionDAGNodes.h
450–456

Yes, just one that got away.

1818–1819

I think that was overaggressive pruning on my part, brought them back.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5086–5091

Personally I like it as a place to hang the comment and don't mind the magic.

I had to make the helper a member function of SelectionDAG, because the nodes' constructors are all private. I wasn't wild about that, but then I saw that we already had a function there for creating SDNodes. So I think it fits ok.

chandlerc accepted this revision.Aug 23 2016, 10:16 AM
chandlerc edited edge metadata.

Two tiny points below but LGTM with those fixed. Thanks for doing this it's a huge improvement!

llvm/include/llvm/CodeGen/SelectionDAG.h
279 ↗(On Diff #69008)

Three slashes for doxygen

286 ↗(On Diff #69008)

Make this static?

This revision is now accepted and ready to land.Aug 23 2016, 10:16 AM
This revision was automatically updated to reflect the committed changes.
jlebar marked 2 inline comments as done.
ahatanak added inline comments.
llvm/trunk/include/llvm/CodeGen/SelectionDAGNodes.h
1103 ↗(On Diff #69014)

I think this assert is not correct for some targets. sizeof(SDNodeBits) is equal to 1 if the target is "-arch armv7" because the alignment of class SDNodeBitfields is 1-byte. If you compile the following code with "-arch armv7", you'll see function foo1 returns 1.

class S1 {
  short HasDebugValue : 1;
  short IsMemIntrinsic : 1;
};

unsigned foo1() {
  S1 s1;
  return sizeof(s1);
}

Justin, I have to fix this soon since there is an internal bot that is failing the assert, which I suspect is happening because sizeof(SDNodeBits) returns 1. It doesn't look like the correctness of the code depends on sizeof(SDNodeBits) being equal to sizeof(unit16_t), so I'll just remove the assert or replace "==" with "<=".

Fixed the static_assert in r279797. Feel free to change the message or remove the assert if you want. I'll be watching the bot to see if it passes the static_assert.