Many target lowerings copy-paste the code to test SDValues for known constants.
This code can instead be shared in the base class, and reused in the targets.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I agree that the code here could be shared.
However TargetLoweringBase provides an interface to customizing the lowering process for a specific target. I do not believe it is the right place for common lowering code.
I don't have a really good answer as there is no existing header/class that I would describe as containing shared utility functions for lowering
and adding a new header just for these 4 functions here seems overkill.
The best thing I can think of right now is adding something like "static bool isOneConstant(SDValue Value)" to the ConstantSDNode class.
That would require qualifying each invocation with a ConstantSDNode::, which is a bit of a mouthful...
Would it be reasonable to define a new class in TargetLowering.h with the four static methods, and to inherit the six affected TargetLowerings, as well as DAGCombiner, from the new class?
Then add them as a normal functions (outside of the ConstantSDNode class). Adding new classes and inheritance is overkill for those 4 functions.
LGTM, good to commit with the nitpicks below addressed. Thanks for working on this!
include/llvm/CodeGen/SelectionDAGNodes.h | ||
---|---|---|
1497–1500 ↗ | (On Diff #41135) | Even though this whole file lacks a lot of doxygen comments, we can do better when we add code. Please add some simple comments similar to /// Returns true if \p V is a ConstantSDNode with value 0. |
lib/Target/X86/X86ISelLowering.cpp | ||
13885–13887 ↗ | (On Diff #41135) | Does not need {} any longer. |
14851–14853 ↗ | (On Diff #41135) | Does not need {} any longer. |
15336–15339 ↗ | (On Diff #41135) | Does not need {} any longer. |
15381–15385 ↗ | (On Diff #41135) | Does not need {} any longer. |