[PATCH] D106649: [RISCV] Add tests showing missed vector saturating add/sub combines
Fraser Cormack via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 27 01:18:13 PDT 2021
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
More information about the llvm-commits
mailing list