[PATCH] D118461: [AMDGPU] Introduce new ISel combine for trunc-slr patterns

[Thomas Symalla via Phabricator via llvm-commits]

tsymalla added a comment.

In D118461#3289697 <https://reviews.llvm.org/D118461#3289697>, @foad wrote:

> In D118461#3289676 <https://reviews.llvm.org/D118461#3289676>, @tsymalla wrote:
>
>> In D118461#3286686 <https://reviews.llvm.org/D118461#3286686>, @foad wrote:
>>
>>> I'm still not sure that we need this, if the xor can be cleaned up earlier. Does D118623 <https://reviews.llvm.org/D118623> help?
>>
>> Jay, unfortunately it doesn't help. I tried your patch out, but for my test case, the matcher won't apply as there is no XOR in the LLVM IR, it only gets created as SDag node. By the way, your change relates to the function "buildConditions" in the comments which should be "insertConditions".
>> However, the idea here is not to remove the XOR, but to remove an additional VALU which gets created by translating the TRUNCATE and the AND in the MIR separately instead of handling them as one sequence. Replacing the "setcc ne" with its inverse and not introducing an additional XOR instead might remove the need for this change, but what about possible other cases where this pattern could get matched?
>
> Well there **is** an XOR in the LLVM IR in the test case in your patch! Can you share another test case?
>
> As a rule of thumb, if there is a missed optimisation, I would like to try to fix it as early as possible in the pass pipeline. Otherwise SelectionDAG has to get more and more complicated, to try to optimise things that should really have been cleaned up in the IR before instruction selection.

I think I misunderstood what you were saying. The pattern that gets matched here is inside the entry block of both functions. The XOR in the second test case (uniform) is there to prevent the truncate in the SDag from being optimized away and has no semantic relevance for the actual test. BTW, your patch still doesn't apply for me in this case. I agree with optimizing the XOR.
Thanks for your additional comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118461