[PATCH] D94730: [RISCV] Add DAG combine to turn (setcc X, 1, setne) -> (setcc X, 0, seteq) if we can prove X is 0/1.

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.

LGTM



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1853
   }
+  case ISD::SETCC: {
+    // (setcc X, 1, setne) -> (setcc X, 0, seteq) if we can prove X is 0/1.
----------------
craig.topper wrote:
> lenary wrote:
> > Is this not a general purpose combine if you have the same boolean contents? Or does it hinder optimisations on other platforms with a wider variety of condition codes?
> I tried to make it generic and failed a bunch of AMDGPU test. The majority where an assertion in SITargetLowering::LowerBRCOND that it expects control flow intrinsics results to be used by a setne with 1. Which this patch broke. I restricted the code to ignore setcc's with MVT::i1 inputs which avoids this assert. This left 12 failing tests.
> 
> Some of those tests got worse because SIOptimizeExecMaskingPreRA::optimizeVcndVcmpPair was optimizing the setne sequence but there's nothing to optimize the code with seteq. The comments specifically mention DAGCombiner::visitBRCOND and rebuildSetCC. This accounts for at least 4 of the 12. I stopped looking after that.
> 
> The was one test changed on AArch64 but it didn't reduce instruction count. A few tests changed on X86. One decreased instruction count. One went from a couple scalar instruction to some vector sequence(the IR contained vectors that were being simplified). Not sure what happened.
Ok, cool.

I think it might be worth proposing as a target-independent patch and seeing what people say, but that shouldn't block us landing this to the RISC-V target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94730