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

[Chandler Carruth via llvm-commits]

chandlerc 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; }
+
----------------
jlebar wrote:
> 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.
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.


https://reviews.llvm.org/D23036