Richard Sandiford <richard.sandif...@arm.com> 于2024年6月5日周三 23:20写道：
>
> YunQiang Su <s...@gcc.gnu.org> writes:
> > Richard Sandiford <richard.sandif...@arm.com> 于2024年6月5日周三 22:14写道：
> >>
> >> YunQiang Su <s...@gcc.gnu.org> writes:
> >> > PR target/113179.
> >> >
> >> > In `store_bit_field_using_insv`, we just use SUBREG if value_mode
> >> >>= op_mode, while in some ports, a sign_extend will be needed,
> >> > such as MIPS64:
> >> > If either GPR rs or GPR rt does not contain sign-extended 32-bit
> >> > values (bits 63..31 equal), then the result of the operation is
> >> > UNPREDICTABLE.
> >> >
> >> > The problem happens for the code like:
> >> > struct xx {
> >> > int a:4;
> >> > int b:24;
> >> > int c:3;
> >> > int d:1;
> >> > };
> >> >
> >> > void xx (struct xx *a, long long b) {
> >> > a->d = b;
> >> > }
> >> >
> >> > In the above code, the hard register contains `b`, may be note well
> >> > sign-extended.
> >> >
> >> > gcc/
> >> > PR target/113179
> >> > * expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
> >> > needed.
> >> >
> >> > gcc/testsuite
> >> > PR target/113179
> >> > * gcc.target/mips/pr113179.c: New tests.
> >> > ---
> >> > gcc/expmed.cc | 12 +++++++++---
> >> > gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
> >> > 2 files changed, 27 insertions(+), 3 deletions(-)
> >> > create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c
> >> >
> >> > diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> >> > index 4ec035e4843..6a582593da8 100644
> >> > --- a/gcc/expmed.cc
> >> > +++ b/gcc/expmed.cc
> >> > @@ -704,9 +704,15 @@ store_bit_field_using_insv (const extraction_insn
> >> > *insv, rtx op0,
> >> > }
> >> > else
> >> > {
> >> > - tmp = gen_lowpart_if_possible (op_mode, value1);
> >> > - if (! tmp)
> >> > - tmp = gen_lowpart (op_mode, force_reg (value_mode,
> >> > value1));
> >> > + if (targetm.mode_rep_extended (op_mode, value_mode))
> >> > + tmp = simplify_gen_unary (TRUNCATE, op_mode,
> >> > + value1, value_mode);
> >> > + else
> >> > + {
> >> > + tmp = gen_lowpart_if_possible (op_mode, value1);
> >> > + if (! tmp)
> >> > + tmp = gen_lowpart (op_mode, force_reg (value_mode,
> >> > value1));
> >> > + }
> >> > }
> >> > value1 = tmp;
> >> > }
> >>
> >> I notice this patch is already applied. Was it approved? I didn't
> >> see an approval in my feed or in the archives.
> >>
> >
> > Sorry. I was supposed that it only effects MIPS targets since only MIPS
> > defines
> > targetm.mode_rep_extended
> >
> >> Although it might not make any difference on current targets,
> >> I think the conditional should logically be based on
> >> TRULY_NOOP_TRUNCATION(_MODES_P) rather than targetm.mode_rep_extended.
> >>
> >> TRULY_NOOP_TRUNCATION is a correctness question: can I use subregs
> >> to do this truncation? targetm.mode_rep_extended is instead an
> >> optimisation question: can I assume that a particular extension is free?
> >> Here we're trying to avoid a subreg for correctness, rather than trying
> >> to take advantage of a cheap extension.
> >>
> >> So I think the code should be:
> >>
> >> if (GET_MODE_SIZE (value_mode) < GET_MODE_SIZE (op_mode))
> >> {
> >> tmp = simplify_subreg (op_mode, value1, value_mode, 0);
> >> if (! tmp)
> >> tmp = simplify_gen_subreg (op_mode,
> >> force_reg (value_mode, value1),
> >> value_mode, 0);
> >> }
> >> else if (GET_MODE_SIZE (op_mode) < GET_MODE_SIZE (value_mode)
> >> && !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode))
> >
> > In fact I don't think so. For other targets besides MIPS, it is safe even
> > !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode)
> > as INS instruction may use the low part of a register safely.
>
> Not sure what you mean. The change above is no-op for targets other
> than MIPS and SH5 (now removed). But I think it's the correct way of
> representing the restriction on MIPS and SH5.
>
> >
> > It is only not true on MIPS ISA documents as
> > If either GPR rs or GPR rt does not contain sign-extended 32-bit
> > values (bits 63..31 equal), then the result of the operation is
> > UNPREDICTABLE.
>
> Right. The reason that TRULY_NOOP_TRUNCATION_MODES_P (SImode, DImode)
> is false for MIPS isn't that a subreg is impossible on MIPS targets.
> It's that the MIPS port needs to ensure that SImode values are stored
> in sign-extended form in order to avoid the problem above. So a
> truncation needs to be a sign-extension.
>
Thanks for your explanation. NVPTX defines TRULY_NOOP_TRUNCATION_MODES_P
to always false.
and
bool
gcn_truly_noop_truncation (poly_uint64 outprec, poly_uint64 inprec)
{
return ((inprec <= 32) && (outprec <= inprec));
}
I am worrying that we may break nvptx/amdgcn as I have little
knowledge about them.
> > It is very annoying. I haven't noticed a similar problem on any other
> > ISA documents.
> > In fact I don't know any real MIPS hardware that is "UNPREDICTABLE" in
> > this case.
>
> I think the Broadcom SB-1 took advantage of the freedom.
>
> Thanks,
> Richard
