[PATCH] D106649: [RISCV] Add tests showing missed vector saturating add/sub combines

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked an inline comment as done.
frasercrmck added inline comments.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/combine-sats.ll:83
+; RV32-NEXT:    vsub.vv v25, v8, v9
+; RV32-NEXT:    vmsltu.vv v0, v8, v25
+; RV32-NEXT:    vsetivli zero, 4, e32, m1, ta, mu
----------------
craig.topper wrote:
> I assume this gets expanded because the legalizer is looking for UMIN/UMAX to be Legal rather than Custom?
Yeah, pretty much. I think this is a pretty weird path to take, at least for our target. The `sub (umin)` is being unconditionally combined to `usubsat` (pre legalization) and then the `usubsat` is being legalized. The legalizer checks whether `umin` is legal, which isn't not for fixed vectors, so it's expanded to `usubo` (and some other ops) which is in turn legalized to a `sub` and `setcc`.

I wonder if this should be fixed. Checking "legal or custom" in the "addsubsat" expansion seems like the right fix in principle. It was actually "legal or custom" until fairly recently -- D91876 -- when some X86 combine was made generic. I don't understand the X86 target well enough to know if this is a significant obstacle to overcome.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106649