[PATCH] D89390: [TargetLowering] Update optimization to check for boolean content

Mon Oct 19 13:41:14 PDT 2020

ErikHogeman added a comment.

In D89390#2338787 <https://reviews.llvm.org/D89390#2338787>, @spatel wrote:

> In D89390#2337357 <https://reviews.llvm.org/D89390#2337357>, @ErikHogeman wrote:
>
>> The original IR that triggered this issue is very large, and unfortunately for a target not checked in upstream, so cannot be used as-is for a test. I have tried cutting it down a bit and keep the relevant parts but I have not been able to get the bug to trigger, either the SETCC is rewritten in some way early or it stays as an i1 type and doesn't trigger the bug. I am also not too familiar with nvptx so not sure what conditions would trigger the SETCC to be rewritten to a larger type. I spent a bit more time trying to cut down the original problematic IR but I'm struggling to reproduce it.
>
> Thanks for experimenting with it. Let's just proceed with what we have.
>
>> Sorry just to clarify that I understood you correctly, would you prefer that I add a test with the nvptx code using a SETCC with i1 type, or would you rather I change the patch to not abort on i1 types since that resulted in more instructions in the example you posted?
>> Uploading this example as a test works for me, but I'm slightly concerned that it seems to generate more instructions now in a case where it should not have to do that.
>
> IIUC, we can do it as 2 patches: (1) add the boolean content clause to cause the extra instructions to show up and (2) add the i1 clause to restore it to optimal. We end up where we started for nvptx, but we still have commit history and this review to document the logic.

Alright, that sounds good to me. Thanks a lot for the feedback.
Then I will update this diff with the example from earlier as an nvptx test, then once this has been committed I will open a second diff (adding you again), updating the patch to check for the i1 case, and update the test for that as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89390