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

[Erik Hogeman via Phabricator via llvm-commits]

ErikHogeman added a comment.

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

> In D89390#2331909 <https://reviews.llvm.org/D89390#2331909>, @ErikHogeman wrote:
>
>> In D89390#2331034 <https://reviews.llvm.org/D89390#2331034>, @spatel wrote:
>>
>>> Looks like we have a couple of in-tree targets where this will make a difference, so we should add a test.
>>>
>>> Try something like this with -mtriple=nvptx ?
>>>
>>>   define i32 @pow2_mask_cmp(i32 %x) {
>>>     %a = and i32 %x, 8
>>>     %cmp = icmp ne i32 %a, 0
>>>     %r = zext i1 %cmp to i32
>>>     ret i32 %r
>>>   }
>>
>> Thanks for your comment. I already spent quite some time trying write a test, it seems pretty difficult to write a small one. I think in your suggested example we still end up getting correct code when this transform triggers, the generated code produces 0 or 1 but that's expected since we are zero extending the i1 result I guess? We do end up generating slightly different code with this patch, so I could write a test that fails before and passes afterwards, but the new code still produces 0 or 1, so not sure that would really be a valid test case then, since the original code is still technically correct?
>
> I agree that the best case would demonstrate a miscompile, but if my suggestion still shows a diff, that's better than nothing. We want to make sure that the code doesn't accidentally change back without someone noticing. If you add the test with a code comment describing what we have discussed here, it should meet the basic need. Even better - add the test first to show what we currently generate, then apply this patch and update/rebase the FileCheck lines. Let me know if that makes sense and/or you need help creating/committing the test file.

Alright, that sounds good to me. I will give that a try then, and let you know if I have any issues. Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89390