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

[Justin Lebar via llvm-commits]

jlebar added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:408-411
@@ +407,6 @@
+
+    bool hasDebugValue() const { return _HasDebugValue; }
+    void setHasDebugValue(bool V) { _HasDebugValue = V; }
+    bool isMemIntrinsic() const { return _IsMemIntrinsic; }
+    void setIsMemIntrinsic(bool V) { _IsMemIntrinsic = V; }
+
----------------
chandlerc wrote:
> 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.
> 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.


https://reviews.llvm.org/D23036