[PATCH] D128080: [SDAG] convert sub from (Pow2-1) constant into xor

Sat Jun 18 07:07:05 PDT 2022

spatel added reviewers: arsenm, craig.topper.
spatel added subscribers: craig.topper, arsenm.
spatel added a comment.
Herald added subscribers: StephenFan, wdng.

In D128080#3594019 <https://reviews.llvm.org/D128080#3594019>, @spatel wrote:

> I also tracked down where this happens in x86 codegen:
> https://github.com/llvm/llvm-project/blob/cd64a427efa0baaf1bb7ae624d4301908afc07f7/llvm/lib/Target/X86/X86InstrCompiler.td#L1535
>
> So this fold and the InstCombine that I copied from are actually over-constrained. We don't need a low-bit-mask constant:
> https://alive2.llvm.org/ce/z/OUm6N_

And digging further, that change was added with D48557 <https://reviews.llvm.org/D48557>, but that was a target-limited form of the original patch D48529 <https://reviews.llvm.org/D48529>...and that was the better version of this patch! :)

I applied that patch today, and the PowerPC regression is no longer present, but there's a different test with an extra load immediate. That seems like a minor regression that could be fixed easily.

The AMDGPU diff seen here was considered a regression then, so I'm assuming that's still true now.

There are now RISCV tests with diffs that look like real improvements in test/CodeGen/RISCV/atomic-signext.ll and /test/CodeGen/RISCV/atomic-rmw.ll:

  -; RV32IA-NEXT:    li a5, 24
  -; RV32IA-NEXT:    sub a3, a5, a3
  +; RV32IA-NEXT:    xori a3, a3, 24

@arsenm @craig.topper - given that we have almost this transform in IR as a canonicalization, do we still want to make this target-specific for DAGCombiner?

================
Comment at: llvm/test/CodeGen/AMDGPU/ds-sub-offset.ll:112
 ; CI-NEXT:    v_lshlrev_b32_e32 v0, 2, v0
-; CI-NEXT:    v_sub_i32_e32 v0, vcc, 0, v0
+; CI-NEXT:    v_xor_b32_e32 v0, 0xffff, v0
 ; CI-NEXT:    v_mov_b32_e32 v1, 13
----------------
This diff was called a code-size regression in D48529.

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

https://reviews.llvm.org/D128080