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

[Justin Lebar via llvm-commits]

jlebar added a comment.

Thank you for the review, Chandler.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:450-456
@@ +449,9 @@
+
+    ISD::LoadExtType extTy() const {
+      return static_cast<ISD::LoadExtType>(ExtTy_);
+    }
+    void setExtTy(ISD::LoadExtType V) {
+      ExtTy_ = static_cast<uint16_t>(V);
+      assert(ExtTy_ == V && "Value truncated");
+    }
+
----------------
chandlerc wrote:
> 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.)
Yes, just one that got away.

================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:1818-1819
@@ -1817,4 +1874,1 @@
-    assert(getExtensionType() == ETy && "LoadExtType encoding error!");
-    assert(readMem() && "Load MachineMemOperand is not a load!");
-    assert(!writeMem() && "Load MachineMemOperand is a store!");
   }
----------------
chandlerc wrote:
> Where did these asserts go? I may just be missing the reason they're already covered, sorry if so...
I think that was overaggressive pruning on my part, brought them back.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5086-5091
@@ -5103,5 +5085,8 @@
   ID.AddInteger(MemVT.getRawBits());
-  ID.AddInteger(encodeMemSDNodeFlags(ExtType, AM, MMO->isVolatile(),
-                                     MMO->isNonTemporal(),
-                                     MMO->isInvariant()));
+  // The compiler can reduce this expression to a constant iff we pass an empty
+  // DebugLoc.  Thankfully, the debug location doesn't have any bearing on the
+  // subclass data.
+  ID.AddInteger(
+      LoadSDNode(dl.getIROrder(), DebugLoc(), VTs, AM, ExtType, MemVT, MMO)
+          .getRawSubclassData());
   ID.AddInteger(MMO->getPointerInfo().getAddrSpace());
----------------
chandlerc wrote:
> 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.
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.


https://reviews.llvm.org/D23036