This is an archive of the discontinued LLVM Phabricator instance.

Expose isXxxConstant() functions from TargetLowering base class (NFC)
ClosedPublic

Authored by tyomitch on Nov 24 2015, 4:03 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

tyomitch updated this revision to Diff 41021.Nov 24 2015, 4:03 AM
tyomitch retitled this revision from to Expose isXxxConstant() functions from TargetLowering base class (NFC).
tyomitch updated this object.
tyomitch added reviewers: MatzeB, andreadb.
tyomitch added a subscriber: llvm-commits.
MatzeB requested changes to this revision.Nov 24 2015, 12:26 PM
MatzeB edited edge metadata.

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.

This revision now requires changes to proceed.Nov 24 2015, 12:26 PM

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.

Thanks Matthias!

Do you have any suggestions where to put it?

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.

Thanks Matthias!

Do you have any suggestions where to put it?

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.

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?

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.

tyomitch updated this revision to Diff 41135.Nov 25 2015, 5:30 AM
tyomitch edited edge metadata.

Updated as discussed

MatzeB accepted this revision.Nov 25 2015, 11:02 AM
MatzeB edited edge metadata.

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.

This revision is now accepted and ready to land.Nov 25 2015, 11:02 AM
This revision was automatically updated to reflect the committed changes.