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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

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.



================
Comment at: llvm/lib/Target/AMDGPU/SIInstructions.td:2290
+// v_and_b32_e64 $a, (1 << $b), $a
+// v_cmp_eq_u32_e64 $a, (1 << $b), $a 
+
----------------
`v_cmp_ne_u32_e64 $a, 0, $a` is probably better because 0 is always an inline constant, but `1 << $b` might not be.


================
Comment at: llvm/test/CodeGen/AMDGPU/dagcombine-lshr-and-cmp.ll:1
+; RUN: llc -march=amdgcn -mtriple=amdgcn-- -stop-after=amdgpu-isel -verify-machineinstrs -O0 < %s | FileCheck -check-prefix=GCN %s
+
----------------
Please auto-generate the checks with utils/update_llc_test_checks.py and pre-commit this test with the old codegen, so that this patch will clearly show how the codegen changes.


================
Comment at: llvm/test/CodeGen/AMDGPU/dagcombine-lshr-and-cmp.ll:41
+}
\ No newline at end of file

----------------
"No newline at end of file" :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118461