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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

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

> In D69326#1736956 <https://reviews.llvm.org/D69326#1736956>, @spatel wrote:
>
> > 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.
>
>
> Some minutes ago I commited and pushed a minor change (added a comment to the MSP430 target), and it worked. So it seems to be all right.  I just directly pushed my local change to origin/master, I assume this is the way to do it. Since this was only a test commit I also assume nothing else must be done.
>
> For the real thing, I suppose I will have to manually update Phabricator status after that, is this right?


https://llvm.org/docs/Phabricator.html#committing-a-change
(or see the section on arcanist)

>> 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.
> 
> I will go for the second option as it seems the most appropriate to me. But I need to ask a couple of questions.
> 
> - In which file should I include the "static cl::opt<bool>" variable/flag?  Since it's "static" I assume it's in the same file it is used, so in this particular case it would go to "MSP430ISelLowering.cpp" which currently has no other variable like this. Is this right?.
> - What else should I implement to get the flag recognised by the command line?

Yes, I was imagining that you can add the cl::opt to MSP430ISelLowering.cpp and use it within an override of isLegalICmpImmediate(). You can find examples at the top of AArch64ISelLowering.cpp, X86ISelLowering.cpp, and probably other targets. You don't need to do anything special to recognize the flag on the command-line.


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

https://reviews.llvm.org/D69326