[PATCH] D69326: [TargetLowering][DAGCombine][MSP430] Shift Amount Threshold in DAGCombine (3)

Thu Nov 7 06:09:24 PST 2019

spatel added a comment.

In D69326#1736386 <https://reviews.llvm.org/D69326#1736386>, @joanlluch wrote:

> @spatel  Thanks for reviewing and commiting my previous patches on this subject. I have now been given commit access. I have not tried yet, but it should be good.

Usually, a first commit will fix a code comment with a spelling error or formatting problem and be "NFC" (no-functional-change). Feel free to try that to make sure things are working.

> I still need this to be reviewed, however in this case I am unsure about how to present test cases because there's no trunk target that directly benefits from this particular patch. I updated the description summary to explain this. I think that this should be commited to trunk because it fixes an actual case of the general LLVM issue that affects targets meeting the conditions. I can present the example of an out-of-trunk target but I understand that this is not particularly helpful. I can also copy/paste parts or the SelectionDAG debug output to at least show what's the difference before and after applying the patch.
> 
> Any suggestions on what to do?

We have 2 options:

1. Accept the patch based on inspection
2. Add a debug flag for MSP430 that is used to force isLegalICmpImmediate() to true/false and set that flag in a test file. Look for variables of the form "static cl::opt<bool>" as an example.

The first option is easier of course, but then we risk breaking the code sometime in the future by accident. The second option is a bit more work, but it is safer.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69326/new/

https://reviews.llvm.org/D69326