On Tue, 18 Feb 2025 13:48:02 -0700, Jeff Law wrote:
>
>
> On 2/18/25 4:12 AM, Jin Ma wrote:
> > We overlooked the side effects of the rounding mode in the pattern,
> > which can impact the result of float_extend and lead to incorrect
> > optimizations in the final program. This issue likely affects nearly
> > all similar patterns that involve rounding modes, and the tests in
> > this patch only highlight one example. It seems challenging to address,
> > and I only implemented a simple fix, which is not a good way to solve
> > the problem.
> >
> > Any comments on this?
> >
> > gcc/ChangeLog:
> >
> > * config/riscv/vector-iterators.md (UNSPEC_VRM): New.
> > * config/riscv/vector.md: Use UNSPEC for float_extend.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/riscv/rvv/base/bug-11.c: New test.
> So as Kito note, the insn you changed already has a reference to the FRM
> it needs -- kept in operands[9]. It seems like your patch, while fixing
> the bug, more likely does so by accident rather than by design.
>
> What I see when I look at the dump files is a deeper issue.
>
>
> In the .expand dump we have:
>
> > (insn 17 16 18 2 (set (reg:HF 147)
> > (const_double:HF 0.0 [0x0.0p+0])) "j.c":14:24 -1
> > (nil))
> > (insn 18 17 19 2 (set (reg/v:RVVMF2SF 141 [ vreg ])
> > (if_then_else:RVVMF2SF (unspec:RVVMF64BI [
> > (reg/v:RVVMF64BI 138 [ vmask ])
> > (const_int 1 [0x1])
> > (const_int 0 [0])
> > (const_int 2 [0x2])
> > (const_int 0 [0])
> > (const_int 2 [0x2])
> > (reg:SI 66 vl)
> > (reg:SI 67 vtype)
> > (reg:SI 69 frm)
> > ] UNSPEC_VPREDICATE)
> > (minus:RVVMF2SF (reg/v:RVVMF2SF 140 [ vreg_memory ])
> > (float_extend:RVVMF2SF (vec_duplicate:RVVMF4HF (reg:HF
> > 147))))
> > (reg/v:RVVMF2SF 140 [ vreg_memory ]))) "j.c":14:24 -1
> > (nil))
>
>
>
> Insn 18 does the subtraction with the adjusted rounding mode. So far,
> so good. Things look fine at the start of cse1. But if we look at the
> end of cse1 we have:
>
> > (insn 17 16 18 2 (set (reg:HF 147)
> > (const_double:HF 0.0 [0x0.0p+0])) "j.c":14:24 136 {*movhf_hardfloat}
> > (nil))
> > (insn 18 17 19 2 (set (reg/v:RVVMF2SF 141 [ vreg ])
> > (reg/v:RVVMF2SF 140 [ vreg_memory ])) "j.c":14:24 2786
> > {*movrvvmf2sf_fract}
> > (expr_list:REG_DEAD (reg:HF 147)
> > (expr_list:REG_DEAD (reg/v:RVVMF2SF 140 [ vreg_memory ])
> > (expr_list:REG_DEAD (reg/v:RVVMF64BI 138 [ vmask ])
> > (expr_list:REG_DEAD (reg:SI 69 frm)
> > (nil))))))
>
>
> Note how CSE replace the arithmetic with a simple copy. At this point
> things are broken.
>
> I don't see how CSE can make the right decision here; we don't expose
> rounding modes this early and thus CSE has no way to know it can't make
> that kind of replacement.
>
> You patch kindof works, but it seems to me it's more accident than
> design and that we need to fix this in a more general manner.
>
> The natural question is what do other targets do when the rounding mode
> gets changed. I'm guessing its exposed as a unspec set before the RTL
> optimizers run.
I apologize for not explaining things more clearly. I also discovered that
the issue is caused by CSE. I think that during the substitution process,
CSE recognized the syntax of if_then_else and concluded that the expressions
in the "then" and "else" branches are equivalent, resulting in both yielding
(reg/v:RVVMF2SF 140 [ vreg_memory ]):
(minus:RVVMF2SF (reg/v:RVVMF2SF 140 [ vreg_memory ])
(float_extend:RVVMF2SF (vec_duplicate:RVVMF4HF (const_double:HF 0.0
[0x0.0p+0]))))
is considered equivalent to:
(reg/v:RVVMF2SF 140 [ vreg_memory ])
Clearly, there wasn’t a deeper consideration of the fact that float_extend
requires
a rounding mode(frm). Therefore, I attempted to use UNSPEC in the pattern to
inform
CSE that we have a rounding mode.
As I mentioned before, this may not be a good solution, as it risks missing
other
optimization opportunities. As you pointed out, we need a more general approach
to fix it. Unfortunately, while I’m still trying to find a solution, I currently
don't have any other good ideas.
Best regards,
Jin Ma
> jeff
