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

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

Mostly minor stuff. This is pretty close I think.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:415
@@ +414,3 @@
+    friend class ConstantSDNode;
+
+    uint16_t : NumSDNodeBits;
----------------
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.

================
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");
+    }
+
----------------
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.)

================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:1202-1203
@@ +1201,4 @@
+    MemSDNodeBits.Ordering = static_cast<uint16_t>(SuccessOrdering);
+    assert(MemSDNodeBits.Ordering == static_cast<uint16_t>(SuccessOrdering) &&
+           "Value truncated");
+    MemSDNodeBits.SynchScope = static_cast<uint16_t>(SynchScope);
----------------
Would the form using the accessor have the same effect:

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

================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:1205
@@ -1144,1 +1204,3 @@
+    MemSDNodeBits.SynchScope = static_cast<uint16_t>(SynchScope);
+    assert(MemSDNodeBits.SynchScope == SynchScope && "Value truncated");
     this->FailureOrdering = FailureOrdering;
----------------
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.

================
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!");
   }
----------------
Where did these asserts go? I may just be missing the reason they're already covered, sorry if so...

================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:1846-1847
@@ -1845,4 +1897,1 @@
-    assert(isTruncatingStore() == isTrunc && "isTrunc encoding error!");
-    assert(!readMem() && "Store MachineMemOperand is a load!");
-    assert(writeMem() && "Store MachineMemOperand is not a store!");
   }
----------------
Just marking the other place where asserts seem to be lost.

================
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());
----------------
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.


https://reviews.llvm.org/D23036