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

Thu Nov 7 09:42:32 PST 2019

joanlluch added a comment.

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?

> 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?

Thanks in advance.

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

https://reviews.llvm.org/D69326