On Mon, 03 Jun 2024 11:50:54 PDT (-0700), jeffreya...@gmail.com wrote:
On 6/3/24 11:03 AM, Palmer Dabbelt wrote:
+;; Provide a minmax pattern for ifcvt to match.
+(define_insn "*<bitmanip_minmax_cmp_insn>_cmp_<mode>3"
+ [(set (match_operand:X 0 "register_operand" "=r")
+ (if_then_else:X
+ (bitmanip_minmax_cmp_op
+ (match_operand:X 1 "register_operand" "r")
+ (match_operand:X 2 "register_operand" "r"))
+ (match_dup 1)
+ (match_dup 2)))]
+ "TARGET_ZBB"
+ "<bitmanip_minmax_cmp_insn>\t%0,%1,%z2"
+ [(set_attr "type" "<bitmanip_minmax_cmp_insn>")])
This is a bit different than how we're handling the other min/max type
attributes
(define_insn "*<bitmanip_optab><mode>3"
[(set (match_operand:X 0 "register_operand" "=r")
(bitmanip_minmax:X (match_operand:X 1 "register_operand" "r")
(match_operand:X 2 "reg_or_0_operand" "rJ")))]
"TARGET_ZBB"
"<bitmanip_insn>\t%0,%1,%z2"
[(set_attr "type" "<bitmanip_insn>")])
but it looks like it ends up with the same types after all the iterators
(there's some "max vs smax" and "smax vs maxs" juggling, but IIUC it
ends up in the same place). I think it'd be clunkier to try and use all
the same iterators, though, so
Reviewed-by: Palmer Dabbelt <pal...@rivosinc.com>
[I was wondering if we need the reversed, Jeff on the call says we
don't. I couldn't figure out how to write a test for it.]
Right. I had managed to convince myself that we didn't need the
reversed case. I'm less sure now than I was earlier, but I'm also
confident that if the need arises we can trivially handle it. At some
point there's canonicalization of the condition and that's almost
certainly what's making it hard to synthesize a testcase for the
reversed pattern.
Ya, no reason to block merging this on the reversed cases. I just
couldn't figure out if we should add them.
The other thing I pondered was whether or not we should support SImode
min/max on rv64. It was critical for simplifying that abs2 routine in
x264, but I couldn't convince myself it was needed here. So I just set
it aside and didn't mention it.
I think because we only have the DI compares that we wouldn't need the
smaller modes? Maybe we should add them, though, as the hueristics
around DImode-ifying compares are fragile so we might trip over some
long-tail performance issues that would be wacky for users to reason
about.
Either way, also doesn't seem like a reason to block this one.
jeff
