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

[Erik Hogeman via Phabricator via llvm-commits]

ErikHogeman added a comment.

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?

To see this bug I suspect you might need a SETCC DAG node that has a type larger than i1. The case that I saw that made me write this patch was a 32-bit SETCC later used by an arithmetic instruction (SUB). This was constructed by several other DAG transforms triggering first.
I tried to modify this test a bit to hopefully get a similar pattern to trigger, for example changing the zext to a sext hoping that would result in the SETCC being promoted to 32 bits and then triggering the bug, but it didn't work unfortunately.

I can spend some more time if you think it's important to have a test for this, but it seems non-trivial to me to write one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89390