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

Fri Oct 16 05:41:29 PDT 2020

spatel added a comment.

In D89390#2332973 <https://reviews.llvm.org/D89390#2332973>, @ErikHogeman wrote:

> I took at look, and there is one thing I thought I should clarify with you before I push another diff. So the code looks like this before applying this patch:
>
>   ld.param.u32    %r1, [pow2_mask_cmp_param_0];
>   bfe.u32     %r2, %r1, 3, 1;
>   st.param.b32    [func_retval0+0], %r2;
>   ret;
>
> After this patch is applied:
>
>   ld.param.u32    %r1, [pow2_mask_cmp_param_0];
>   and.b32     %r2, %r1, 8;
>   setp.ne.s32     %p1, %r2, 0;
>   selp.u32    %r3, 1, 0, %p1;
>   st.param.b32    [func_retval0+0], %r3;
>   ret;
>
> So it looks like we generate more instructions when disabling this transformation, which is unfortunate.
> But I'm thinking now, this transformation is probably correct for setcc nodes with i1 type regardless of the boolean content, so maybe I should update the patch to only check the boolean contents when the type has a larger bitsize than 1?
> That would make sure we still generate the code as in the first case above, but it would mean I'm back to not being sure how to write a good test for this.
> Any thoughts?

Let's add the nvptx test with current codegen as a preliminary step. That way, we'll at least have better coverage for today's behavior.
Then, it's a question of how to expose the buggy behavior. I'm not familiar with nvptx; can we make a larger test that would still show a diff? Is your motivating example/target public, or can we adapt the larger example to work with nvptx?
There's another way to improve this whole block and probably demonstrate the bug on multiple in-tree targets if you're up for it: fix the TODO to allow vector types.
If none of the above, then we can continue as originally proposed without a regression test.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89390