Page MenuHomePhabricator

[TargetLowering][DAGCombine][MSP430] Shift Amount Threshold in DAGCombine (3)
ClosedPublic

Authored by joanlluch on Oct 22 2019, 3:11 PM.

Details

Summary

Additional filtering of undesired shifts for targets that do not support them efficiently.

Related with D69116 and D69120

Applies the TLI.getShiftAmountThreshold hook to prevent undesired generation of shifts for the following IR code:

define i16 @testShiftBits(i16 %a) {
entry:
  %and = and i16 %a, -64
  %cmp = icmp eq i16 %and, 64
  %conv = zext i1 %cmp to i16
  ret i16 %conv
}

define i16 @testShiftBits_11(i16 %a) {
entry:
  %cmp = icmp ugt i16 %a, 63
  %conv = zext i1 %cmp to i16
  ret i16 %conv
}

define i16 @testShiftBits_12(i16 %a) {
entry:
  %cmp = icmp ult i16 %a, 64
  %conv = zext i1 %cmp to i16
  ret i16 %conv
}

The attached diff file shows the piece code in TargetLowering that is responsible for the generation of shifts in relation to the IR above.

Before applying this patch, shifts will be generated to replace non-legal icmp immediates. However, shifts may be undesired if they are even more expensive for the target.

For all my previous patches in this series (cited above) I added test cases for the MSP430 target. However, in this case, the target is not suitable for showing improvements related with this patch, because the MSP430 does not implement "isLegalICmpImmediate". The default implementation returns always true, therefore the patched code in TargetLowering is never reached for that target. Targets implementing both "isLegalICmpImmediate" and "getShiftAmountThreshold" will benefit from this.

The differential effect of this patch can only be shown for the MSP430 by temporarily implementing "isLegalICmpImmediate" to return false for large immediates. This is simulated with the implementation of a command line flag that was incorporated in D69975

This patch belongs to a initiative to "relax" the generation of shifts by LLVM for targets requiring it

Diff Detail

Event Timeline

joanlluch created this revision.Oct 22 2019, 3:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2019, 3:11 PM
joanlluch edited the summary of this revision. (Show Details)Oct 22 2019, 11:31 PM
lenary added a subscriber: lenary.Oct 28 2019, 9:46 AM
joanlluch updated this revision to Diff 228156.Nov 6 2019, 3:19 PM
joanlluch edited the summary of this revision. (Show Details)

Made sure diff matches latest trunk changes

@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.

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?

Thanks.

spatel added a comment.EditedNov 7 2019, 6:03 AM

@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.

spatel added a comment.Nov 7 2019, 7:47 AM

You will want to rebase this patch either way - I had to do some cleaning just to step through this function:
rG2fdd58c5066f
rG777d1d1d9811

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.

You will want to rebase this patch either way - I had to do some cleaning just to step through this function:
rG2fdd58c5066f
rG777d1d1d9811

Ok, no problem.

spatel added a comment.Nov 7 2019, 2:27 PM

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.

joanlluch planned changes to this revision.Nov 7 2019, 3:46 PM

Following the above conversation, I created D69975 as the base tests before this patch can be committed.
I put the base tests in a separate patch to actually show the differential effects of this one.
I will open this for review after D69975 is reviewed.
Thanks,

joanlluch updated this revision to Diff 228560.Nov 8 2019, 11:52 PM

Updated after D69975 was reviewed and committed.
Now showing the codegen improvements related with this patch

spatel accepted this revision.Nov 10 2019, 6:33 AM

LGTM

This revision is now accepted and ready to land.Nov 10 2019, 6:33 AM
joanlluch edited the summary of this revision. (Show Details)Nov 11 2019, 1:08 AM
This revision was automatically updated to reflect the committed changes.