Re: RFC: Top level configure: Require a minimum version 6.8 texinfo
> I think that is too new. > We still allow building gcc e.g. with GCC 4.8 from ~ 10 years ago and > I think various boxes where people regularly build gcc will have similarly > old other tools. > So, bumping requirement from ~ 20 years old tools to ~ 10 years old tools > might be ok, but requiring ones at most 2 years old will be a nightmare, I agree. Lots of toolchains distributed by CPU vendors are supposed to support CentOS 6, or at least 7.
MIPS: the method of getting GOT address for PIC code
When working on LLVM, I found this problem https://github.com/llvm/llvm-project/issues/64974. Maybe it's time for us to reconsider the way of getting GOT address for PIC code. 1.Background[1]: All of the accessing of global variables and normal function calls needs help from GOT. So normally, the first 3 instructions of a function are to compute the address of GOT. Normally like: lui $gp,%hi(_gp_disp) addiu $gp,$gp,%lo(_gp_disp) addu$gp,$gp,$t9 These 3 instructions load the value of _gp_disp, which is a link-time constant value, and add them with $t9, which normally contains the address of the current function. So, why $t9? The reason is that the pre-R6 MIPS lacks instructions to get the PC value. Thus, the ABI defines that, the caller should call functions with jr/jarl $t9. So the callee can get the address of itself by $t9. 2. What's my proposal? 2.1 For MIPSr6, its has instructions to get the current PC value, so we can use them, which can gain performance improvement. 2.2 For pre-R6, in fact, it can get the value of PC with NAL instruction [2], while the performance will be some worse. For the worst case, the decreasing of performance will be 20%-40% My plan for pre-R6, is to add a non-default option to use NAL to get the address of GOT. Some software, like Linux kernel (for Kalsr support) will need it. Any suggestions? [1] https://refspecs.linuxfoundation.org/elf/mipsabi.pdf [2] https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00087-2B-MIPS64BIS-AFP-6.06.pdf Page 404 -- YunQiang Su
Re: [RFC] Combine zero_extract and sign_extend for TARGET_TRULY_NOOP_TRUNCATION
> > Like I mentioned in the other thread, I think things went wrong when > we generated the subreg in this sign_extend. The operation should > have been a truncate of (reg/v:DI 200) followed by a sign extension > of the result. > Sorry for my misunderstanding. So you mean that in the RTL, for this operation: we should have 3 (insn ) RTX? (zero_extract ) (truncate_64_to_32) (sign_extend_32_to_64) > What piece of code is generating the subreg? > > Thanks, > Richard -- YunQiang Su
Re: [RFC] Combine zero_extract and sign_extend for TARGET_TRULY_NOOP_TRUNCATION
YunQiang Su 于2023年8月3日周四 11:18写道：
>
> PR #104914
>
> On TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)) == true platforms,
> zero_extract (SI, SI) can be sign-extended. So, if a zero_extract (DI,
> DI) following with an sign_extend(SI, DI) can be merged to a single
> zero_extract (SI, SI).
>
The RTL is like:
(insn 10 49 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
(const_int 8 [0x8])
(const_int 0 [0]))
(subreg:DI (reg:QI 202 [ *buf_8(D) ]) 0)) "xx.c":4:29 281 {*insvdi}
(expr_list:REG_DEAD (reg:QI 202 [ *buf_8(D) ])
(nil)))
(insn 11 10 12 2 (set (reg/v:DI 200 [ val ])
(sign_extend:DI (subreg:SI (reg/v:DI 200 [ val ]) 0)))
"xx.c":4:29 238 {extendsidi2}
(nil))
--->
(note 10 49 11 2 NOTE_INSN_DELETED)
(insn 11 10 12 2 (set (zero_extract:SI (subreg:SI (reg/v:DI 200 [ val ]) 0)
(const_int 8 [0x8])
(const_int 0 [0]))
(subreg:SI (reg:QI 202 [ *buf_8(D) ]) 0)) "xx.c":4:29 280 {*insvsi}
(expr_list:REG_DEAD (reg:QI 202 [ *buf_8(D) ])
(nil)))
This is another method to solve #104914.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914
Another method is here:
https://gcc.gnu.org/pipermail/gcc-patches/2023-July/624856.html
aka when generate RTL for zero_extract, we can determine whether it
is SImode. So we can generate the correct zero_extract at the first time,
aka in the expand pass.
Any idea about which method is better?
> gcc/ChangeLog:
> PR: 104914.
> * combine.cc (try_combine): Combine zero_extract (DI, DI) and
> following sign_extend (DI, SI) for
> TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)) == true.
> (subst): Allow replacing reg(DI) with subreg(SI (reg DI))
> if to is SImode and from is DImode for
> TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)) == true.
>
> gcc/testsuite/ChangeLog:
> PR: 104914.
> * gcc.target/mips/pr104914.c: New testcase.
> ---
> gcc/combine.cc | 88
> gcc/testsuite/gcc.target/mips/pr104914.c | 17 +
> 2 files changed, 90 insertions(+), 15 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/mips/pr104914.c
>
> diff --git a/gcc/combine.cc b/gcc/combine.cc
> index e46d202d0a7..701b7c33b17 100644
> --- a/gcc/combine.cc
> +++ b/gcc/combine.cc
> @@ -3294,15 +3294,64 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn
> *i1, rtx_insn *i0,
>n_occurrences = 0; /* `subst' counts here */
>subst_low_luid = DF_INSN_LUID (i2);
>
> - /* If I1 feeds into I2 and I1DEST is in I1SRC, we need to make a unique
> -copy of I2SRC each time we substitute it, in order to avoid creating
> -self-referential RTL when we will be substituting I1SRC for I1DEST
> -later. Likewise if I0 feeds into I2, either directly or indirectly
> -through I1, and I0DEST is in I0SRC. */
> - newpat = subst (PATTERN (i3), i2dest, i2src, false, false,
> - (i1_feeds_i2_n && i1dest_in_i1src)
> - || ((i0_feeds_i2_n || (i0_feeds_i1_n && i1_feeds_i2_n))
> - && i0dest_in_i0src));
> + /* Try to combine zero_extract (DImode) and sign_extend (SImode to
> DImode)
> +for TARGET_TRULY_NOOP_TRUNCATION. The RTL may look like:
> +
> +(insn 10 49 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
> + (const_int 8 [0x8])
> + (const_int 0 [0]))
> +(subreg:DI (reg:QI 202 [ *buf_8(D) ]) 0)) "xx.c":4:29 278
> {*insvdi}
> +(expr_list:REG_DEAD (reg:QI 202 [ *buf_8(D) ]) (nil)))
> +(insn 11 10 12 2 (set (reg/v:DI 200 [ val ])
> +
> +(sign_extend:DI (subreg:SI (reg/v:DI 200 [ val ]) 0))) 238
> {extendsidi2}
> +(nil))
> +
> +Since these architectures (MIPS64 as an example), the 32bit operation
> +instructions will sign-extend the reuslt to 64bit. The result can
> be:
> +
> +(insn 10 49 11 2 (set (zero_extract:SI (subreg:SI (reg/v:DI 200 [
> val ]) 0)
> + (const_int 8 [0x8])
> + (const_int 0 [0]))
> +(subreg:SI (reg:QI 202 [ *buf_8(D) ]) 0)) "xx.c":4:29 280
> {*insvsi}
> +(expr_list:REG_DEAD (reg:QI 202 [ *buf_8(D) ]) (nil)))
> + */
> + if (i0 == 0 && i1 == 0 && i3 != 0 && i2 != 0 && GET_CODE (i2) == INSN
> + && GET_CODE (i3) == INSN && GET_CODE (PATTERN (i2)) == SET
> + && GET_CODE (PATTERN (i3)) == SET
> + && GET_CODE (SET_DEST (single_set (i2))) == ZERO_EXTRACT
> + && GET_CODE (SET_SRC (single_set (i3))) == SIGN_EXTEND
> + && SUBREG_P (XEXP (SET_SRC (single_set (i3)), 0))
> + && REGNO (SUBREG_REG (XEXP (SET_SRC (single_set (i3)), 0)))
> +== REGNO (SET_DEST (single_set (i3)))
> + && REGNO (XEXP (SET_DEST (single_set (i2)), 0))
> +== REGNO (SET_DEST (single_set (i3)))
> + &&
Re: [PATCH v2] Store_bit_field_1: Use SUBREG instead of REG if possible
Eric Botcazou 于2023年7月19日周三 17:45写道：
>
> > I don't see that. That's definitely not what GCC expects here,
> > the left-most word of the doubleword should be unchanged.
> >
> > Your testcase should be a dg-do-run and probably more like
> >
> > NOMIPS16 int __attribute__((noipa)) test (const unsigned char *buf)
> > {
> > int val;
> > ((unsigned char*))[0] = *buf++;
> > ((unsigned char*))[1] = *buf++;
> > ((unsigned char*))[2] = *buf++;
> > ((unsigned char*))[3] = *buf++;
> > return val;
> > }
> > int main()
> > {
> > int val = 0x01020304;
> > val = test ();
> > if (val != 0x01020304)
> > abort ();
> > }
> >
> > not sure if I got endianess correct. Now, the question is what
> > WORD_REGISTER_OPERATIONS implies for a bitfield insert and what
> > the MIPS ABI says for returning SImode.
>
MIPS N64 ABI uses 2 GPR for integer return values.
If the return value is SImode, the first v0 register is used, and it
must be sign-extended,
aka the bits[64-31] are all same.
Yes, it is same for signed and unsigned int32.
https://irix7.com/techpubs/007-2816-004.pdf
Page 6:
32-bit integer (int) parameters are always sign-extended when passed
in registers,
whether of signed or unsigned type. [This issue does not arise in the
o32-bit ABI.]
> WORD_REGISTER_OPERATIONS must *not* be taken account for bit-fields, see e;g.
> word_register_operation_p:
>
> /* Return true if X is an operation that always operates on the full
>registers for WORD_REGISTER_OPERATIONS architectures. */
>
> inline bool
> word_register_operation_p (const_rtx x)
> {
> switch (GET_CODE (x))
> {
> case CONST_INT:
> case ROTATE:
> case ROTATERT:
> case SIGN_EXTRACT:
> case ZERO_EXTRACT:
> return false;
>
> default:
> return true;
> }
> }
>
> --
> Eric Botcazou
>
>
--
YunQiang Su
Re: [PATCH v2] Store_bit_field_1: Use SUBREG instead of REG if possible
Richard Biener 于2023年7月19日周三 17:23写道：
>
> On Wed, 19 Jul 2023, YunQiang Su wrote:
>
> > Richard Biener ?2023?7?19??? 15:22???
> > >
> > > On Wed, 19 Jul 2023, YunQiang Su wrote:
> > >
> > > > Richard Biener via Gcc-patches ?2023?7?19???
> > > > 14:27???
> > > > >
> > > > > On Wed, 19 Jul 2023, YunQiang Su wrote:
> > > > >
> > > > > > PR #104914
> > > > > >
> > > > > > When work with
> > > > > > int val;
> > > > > > ((unsigned char*))[3] = *buf;
> > > > > > if (val > 0) ...
> > > > > > The RTX mode is obtained from REG instead of SUBREG, which make
> > > > > > D is used instead of . Thus something wrong happens
> > > > > > on sign-extend default architectures, like MIPS64.
> > > > > >
> > > > > > Let's use str_rtx and mode of str_rtx as the parameters for
> > > > > > store_integral_bit_field if:
> > > > > > modes of op0 and str_rtx are INT;
> > > > > > length of op0 is greater than str_rtx.
> > > > > >
> > > > > > This patch has been tested on aarch64-linux-gnu, x86_64-linux-gnu,
> > > > > > mips64el-linux-gnuabi64 without regression.
> > > > >
> > > > > I still think you are "fixing" this in the wrong place. The bugzilla
> > > > > audit trail points to combine and later notes an eventual expansion
> > > > > issue (but for another testcase/target).
> > > > >
> > > > > You have to explain in more detail on what is wrong with the initial
> > > > > RTL on mips.
> > > > >
> > > >
> > > > In the first RTL file, aka xx.c.256r.expand, the zero_extract RTX is
> > > > like
> > > >
> > > > (insn 10 9 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
> > > > (const_int 8 [0x8])
> > > > (const_int 0 [0]))
> > > > (subreg:DI (reg:QI 202) 0)) "../xx.c":4:29 -1
> > > > (nil))
> > > >
> > > > Not, all of the REG are in DImode. On MIPS64, it will expand to `DINS`
> > > > instructions.
> > > > While in fact here, we expect an SImode operation, due to `val` in C
> > > > code is `int`.
> > > >
> > > > With my patch, the RTX will be like:
> > > >
> > > > (insn 10 9 11 2 (set (zero_extract:SI (subreg:SI (reg/v:DI 200 [ val ])
> > > > 0)
> > > > (const_int 8 [0x8])
> > > > (const_int 0 [0]))
> > > > (subreg:SI (reg:QI 202) 0)) "xx.c":4:29 -1
> > > > (nil))
> > >
> > > But if this RTL is correct then the above with DImode is correct as
> > > well and the issue is in the backend definition of the instruction
> > > defining 'DINS'?
> > >
> >
> > I don't think so.
> >
> > (insn 10 9 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
> > ^^
> > (const_int 8 [0x8])
> > (const_int 0 [0]))
> > (subreg:DI (reg:QI 202) 0)) "../xx.c":4:29 -1
> > (nil))
> >
> > This RTL has only info about DI. It doesn't has any info about the
> > real length of
> > `val`. For backend, it has no other choice instead of `DINS`.
> >
> > > > So the operation will be SImode, aka `INS` instruction for MIPS64.
> > > >
> > > > The problem is based on 2 fact/root cause:
> > > > 1. MIPS's `INS` instruction will be always to sign-extension, while
> > > > `DINS` won't
> > > > li $7, 0xff
> > > > li $8, 0
> > > > ins $8,$7,24,8 # set the 24-32 bits of $8 to 0xff.
> > > > The value of $8 will be 0xff ff ff ff ff 00 00 00.
> > >
> > > Bit that's wrong. (set (zero_extract:SI ...) should not affect
> > > bits outside of the indicated range.
> > >
> >
> > In fact, it is how sign-extension arch work.
> > No matter wrong or right, the ISA was/is defined like this.
> >
> > In fact, one MIPS 32 ABI, the same C code will generate the RTL like this,
> > and the 32bit object can still workable on 64bit CPU.
> > That's a smart (or brain-damaged) design.
> >
> > > @findex zero_extract
> > > @item (zero_extract:@var{m} @var{loc} @var{size} @var{pos})
> > > Like @code{sign_extract} but refers to an unsigned or zero-extended
> > > bit-field. The same sequence of bits are extracted, but they
> > > are filled to an entire word with zeros instead of by sign-extension.
> > >
> >
> > That's depending on the definition of `word` here.
> > For `(zero_extract:SI`, I think that the word is limit to the low 32bit of
> > hardware register.
> > Anyway, it won't break ISA without sign-extension by default.
> >
> > Due to the nature of sign-extension ISA, if we don't sign-extension the
> > `int` variable, it will make something wrong.
> >
> > To make it clear: the word `sign extension` here means:
> >the the value of 31bit will be copied to bits [32-63], and
> >the value of bits[0-30] won't be copied.
> > Here is the examples:
> > li $7, 0xff
> > li $8, 0x00 00 ff 00
> > ins $8,$7,16,8
> > ^^
> > The value of $8 will be: 0x 00 00 00 00 00 ff ff 00
> >
> > li $7, 0xff
> > li $8, 0x00 00 ff 00
> > ins $8,$7,24,8
> > ^^
> > The value of $8 will be: 0x ff ff ff ff ff 00 ff 00
>
> But that's INS.
>
> > > Unlike
Re: [PATCH v2] Store_bit_field_1: Use SUBREG instead of REG if possible
I am not sure this patch is best, while I think that I am sure the
initial RTL is not correct,
the initial RTL of ARM64 is like
(insn 8 7 9 2 (set (zero_extract:SI (reg/v:SI 98 [ val ])
^^
(const_int 8 [0x8])
(const_int 0 [0]))
(reg:SI 102)) "xx.c":3:29 -1
(nil))
YunQiang Su 于2023年7月19日周三 16:25写道：
>
> YunQiang Su 于2023年7月19日周三 16:21写道：
> >
> > Richard Biener 于2023年7月19日周三 15:22写道：
> > >
> > > On Wed, 19 Jul 2023, YunQiang Su wrote:
> > >
> > > > Richard Biener via Gcc-patches ?2023?7?19???
> > > > 14:27???
> > > > >
> > > > > On Wed, 19 Jul 2023, YunQiang Su wrote:
> > > > >
> > > > > > PR #104914
> > > > > >
> > > > > > When work with
> > > > > > int val;
> > > > > > ((unsigned char*))[3] = *buf;
> > > > > > if (val > 0) ...
> > > > > > The RTX mode is obtained from REG instead of SUBREG, which make
> > > > > > D is used instead of . Thus something wrong happens
> > > > > > on sign-extend default architectures, like MIPS64.
> > > > > >
> > > > > > Let's use str_rtx and mode of str_rtx as the parameters for
> > > > > > store_integral_bit_field if:
> > > > > > modes of op0 and str_rtx are INT;
> > > > > > length of op0 is greater than str_rtx.
> > > > > >
> > > > > > This patch has been tested on aarch64-linux-gnu, x86_64-linux-gnu,
> > > > > > mips64el-linux-gnuabi64 without regression.
> > > > >
> > > > > I still think you are "fixing" this in the wrong place. The bugzilla
> > > > > audit trail points to combine and later notes an eventual expansion
> > > > > issue (but for another testcase/target).
> > > > >
> > > > > You have to explain in more detail on what is wrong with the initial
> > > > > RTL on mips.
> > > > >
> > > >
> > > > In the first RTL file, aka xx.c.256r.expand, the zero_extract RTX is
> > > > like
> > > >
> > > > (insn 10 9 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
> > > > (const_int 8 [0x8])
> > > > (const_int 0 [0]))
> > > > (subreg:DI (reg:QI 202) 0)) "../xx.c":4:29 -1
> > > > (nil))
> > > >
> > > > Not, all of the REG are in DImode. On MIPS64, it will expand to `DINS`
> > > > instructions.
> > > > While in fact here, we expect an SImode operation, due to `val` in C
> > > > code is `int`.
> > > >
> > > > With my patch, the RTX will be like:
> > > >
> > > > (insn 10 9 11 2 (set (zero_extract:SI (subreg:SI (reg/v:DI 200 [ val ])
> > > > 0)
> > > > (const_int 8 [0x8])
> > > > (const_int 0 [0]))
> > > > (subreg:SI (reg:QI 202) 0)) "xx.c":4:29 -1
> > > > (nil))
> > >
> > > But if this RTL is correct then the above with DImode is correct as
> > > well and the issue is in the backend definition of the instruction
> > > defining 'DINS'?
> > >
> >
> > I don't think so.
> >
> > (insn 10 9 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
> > ^^
> > (const_int 8 [0x8])
> > (const_int 0 [0]))
> > (subreg:DI (reg:QI 202) 0)) "../xx.c":4:29 -1
> > (nil))
> >
> > This RTL has only info about DI. It doesn't has any info about the
> > real length of
> > `val`. For backend, it has no other choice instead of `DINS`.
> >
> > > > So the operation will be SImode, aka `INS` instruction for MIPS64.
> > > >
> > > > The problem is based on 2 fact/root cause:
> > > > 1. MIPS's `INS` instruction will be always to sign-extension, while
> > > > `DINS` won't
> > > > li $7, 0xff
> > > > li $8, 0
> > > > ins $8,$7,24,8 # set the 24-32 bits of $8 to 0xff.
> > > > The value of $8 will be 0xff ff ff ff ff 00 00 00.
> > >
> > > Bit that's wrong. (set (zero_extract:SI ...) should not affect
> > > bits outside of the indicated range.
> > >
> >
> > In fact, it is how sign-extension arch work.
> > No matter wrong or right, the ISA was/is defined like this.
> >
> > In fact, one MIPS 32 ABI, the same C code will generate the RTL like this,
> > and the 32bit object can still workable on 64bit CPU.
> > That's a smart (or brain-damaged) design.
> >
> > > @findex zero_extract
> > > @item (zero_extract:@var{m} @var{loc} @var{size} @var{pos})
> > > Like @code{sign_extract} but refers to an unsigned or zero-extended
> > > bit-field. The same sequence of bits are extracted, but they
> > > are filled to an entire word with zeros instead of by sign-extension.
> > >
> >
> > That's depending on the definition of `word` here.
> > For `(zero_extract:SI`, I think that the word is limit to the low 32bit of
> > hardware register.
> > Anyway, it won't break ISA without sign-extension by default.
> >
> > Due to the nature of sign-extension ISA, if we don't sign-extension the
> > `int` variable, it will make something wrong.
> >
> > To make it clear: the word `sign extension` here means:
> >the the value of 31bit will be copied to bits [32-63], and
> >the value of bits[0-30] won't be copied.
> > Here is the examples:
> > li $7,
Re: [PATCH v2] Store_bit_field_1: Use SUBREG instead of REG if possible
YunQiang Su 于2023年7月19日周三 16:21写道：
>
> Richard Biener 于2023年7月19日周三 15:22写道：
> >
> > On Wed, 19 Jul 2023, YunQiang Su wrote:
> >
> > > Richard Biener via Gcc-patches ?2023?7?19???
> > > 14:27???
> > > >
> > > > On Wed, 19 Jul 2023, YunQiang Su wrote:
> > > >
> > > > > PR #104914
> > > > >
> > > > > When work with
> > > > > int val;
> > > > > ((unsigned char*))[3] = *buf;
> > > > > if (val > 0) ...
> > > > > The RTX mode is obtained from REG instead of SUBREG, which make
> > > > > D is used instead of . Thus something wrong happens
> > > > > on sign-extend default architectures, like MIPS64.
> > > > >
> > > > > Let's use str_rtx and mode of str_rtx as the parameters for
> > > > > store_integral_bit_field if:
> > > > > modes of op0 and str_rtx are INT;
> > > > > length of op0 is greater than str_rtx.
> > > > >
> > > > > This patch has been tested on aarch64-linux-gnu, x86_64-linux-gnu,
> > > > > mips64el-linux-gnuabi64 without regression.
> > > >
> > > > I still think you are "fixing" this in the wrong place. The bugzilla
> > > > audit trail points to combine and later notes an eventual expansion
> > > > issue (but for another testcase/target).
> > > >
> > > > You have to explain in more detail on what is wrong with the initial
> > > > RTL on mips.
> > > >
> > >
> > > In the first RTL file, aka xx.c.256r.expand, the zero_extract RTX is like
> > >
> > > (insn 10 9 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
> > > (const_int 8 [0x8])
> > > (const_int 0 [0]))
> > > (subreg:DI (reg:QI 202) 0)) "../xx.c":4:29 -1
> > > (nil))
> > >
> > > Not, all of the REG are in DImode. On MIPS64, it will expand to `DINS`
> > > instructions.
> > > While in fact here, we expect an SImode operation, due to `val` in C
> > > code is `int`.
> > >
> > > With my patch, the RTX will be like:
> > >
> > > (insn 10 9 11 2 (set (zero_extract:SI (subreg:SI (reg/v:DI 200 [ val ]) 0)
> > > (const_int 8 [0x8])
> > > (const_int 0 [0]))
> > > (subreg:SI (reg:QI 202) 0)) "xx.c":4:29 -1
> > > (nil))
> >
> > But if this RTL is correct then the above with DImode is correct as
> > well and the issue is in the backend definition of the instruction
> > defining 'DINS'?
> >
>
> I don't think so.
>
> (insn 10 9 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
> ^^
> (const_int 8 [0x8])
> (const_int 0 [0]))
> (subreg:DI (reg:QI 202) 0)) "../xx.c":4:29 -1
> (nil))
>
> This RTL has only info about DI. It doesn't has any info about the
> real length of
> `val`. For backend, it has no other choice instead of `DINS`.
>
> > > So the operation will be SImode, aka `INS` instruction for MIPS64.
> > >
> > > The problem is based on 2 fact/root cause:
> > > 1. MIPS's `INS` instruction will be always to sign-extension, while
> > > `DINS` won't
> > > li $7, 0xff
> > > li $8, 0
> > > ins $8,$7,24,8 # set the 24-32 bits of $8 to 0xff.
> > > The value of $8 will be 0xff ff ff ff ff 00 00 00.
> >
> > Bit that's wrong. (set (zero_extract:SI ...) should not affect
> > bits outside of the indicated range.
> >
>
> In fact, it is how sign-extension arch work.
> No matter wrong or right, the ISA was/is defined like this.
>
> In fact, one MIPS 32 ABI, the same C code will generate the RTL like this,
> and the 32bit object can still workable on 64bit CPU.
> That's a smart (or brain-damaged) design.
>
> > @findex zero_extract
> > @item (zero_extract:@var{m} @var{loc} @var{size} @var{pos})
> > Like @code{sign_extract} but refers to an unsigned or zero-extended
> > bit-field. The same sequence of bits are extracted, but they
> > are filled to an entire word with zeros instead of by sign-extension.
> >
>
> That's depending on the definition of `word` here.
> For `(zero_extract:SI`, I think that the word is limit to the low 32bit of
> hardware register.
> Anyway, it won't break ISA without sign-extension by default.
>
> Due to the nature of sign-extension ISA, if we don't sign-extension the
> `int` variable, it will make something wrong.
>
> To make it clear: the word `sign extension` here means:
>the the value of 31bit will be copied to bits [32-63], and
>the value of bits[0-30] won't be copied.
> Here is the examples:
> li $7, 0xff
> li $8, 0x00 00 ff 00
> ins $8,$7,16,8
> ^^
> The value of $8 will be: 0x 00 00 00 00 00 ff ff 00
>
> li $7, 0xff
> li $8, 0x00 00 ff 00
> ins $8,$7,24,8
> ^^
> The value of $8 will be: 0x ff ff ff ff ff 00 ff 00
>
> > Unlike @code{sign_extract}, this type of expressions can be lvalues
> > in RTL; they may appear on the left side of an assignment, indicating
> > insertion of a value into the specified bit-field.
> > @end table
> >
> >
> > > li $7, 0xff
> > > li $8, 0
> > > dins $8,$7,24,8 # set the 24-32 bits of $8 to 0xff.
> > > The value of $8 will be 0x 00
Re: [PATCH v2] Store_bit_field_1: Use SUBREG instead of REG if possible
Richard Biener 于2023年7月19日周三 15:22写道：
>
> On Wed, 19 Jul 2023, YunQiang Su wrote:
>
> > Richard Biener via Gcc-patches ?2023?7?19???
> > 14:27???
> > >
> > > On Wed, 19 Jul 2023, YunQiang Su wrote:
> > >
> > > > PR #104914
> > > >
> > > > When work with
> > > > int val;
> > > > ((unsigned char*))[3] = *buf;
> > > > if (val > 0) ...
> > > > The RTX mode is obtained from REG instead of SUBREG, which make
> > > > D is used instead of . Thus something wrong happens
> > > > on sign-extend default architectures, like MIPS64.
> > > >
> > > > Let's use str_rtx and mode of str_rtx as the parameters for
> > > > store_integral_bit_field if:
> > > > modes of op0 and str_rtx are INT;
> > > > length of op0 is greater than str_rtx.
> > > >
> > > > This patch has been tested on aarch64-linux-gnu, x86_64-linux-gnu,
> > > > mips64el-linux-gnuabi64 without regression.
> > >
> > > I still think you are "fixing" this in the wrong place. The bugzilla
> > > audit trail points to combine and later notes an eventual expansion
> > > issue (but for another testcase/target).
> > >
> > > You have to explain in more detail on what is wrong with the initial
> > > RTL on mips.
> > >
> >
> > In the first RTL file, aka xx.c.256r.expand, the zero_extract RTX is like
> >
> > (insn 10 9 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
> > (const_int 8 [0x8])
> > (const_int 0 [0]))
> > (subreg:DI (reg:QI 202) 0)) "../xx.c":4:29 -1
> > (nil))
> >
> > Not, all of the REG are in DImode. On MIPS64, it will expand to `DINS`
> > instructions.
> > While in fact here, we expect an SImode operation, due to `val` in C
> > code is `int`.
> >
> > With my patch, the RTX will be like:
> >
> > (insn 10 9 11 2 (set (zero_extract:SI (subreg:SI (reg/v:DI 200 [ val ]) 0)
> > (const_int 8 [0x8])
> > (const_int 0 [0]))
> > (subreg:SI (reg:QI 202) 0)) "xx.c":4:29 -1
> > (nil))
>
> But if this RTL is correct then the above with DImode is correct as
> well and the issue is in the backend definition of the instruction
> defining 'DINS'?
>
I don't think so.
(insn 10 9 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
^^
(const_int 8 [0x8])
(const_int 0 [0]))
(subreg:DI (reg:QI 202) 0)) "../xx.c":4:29 -1
(nil))
This RTL has only info about DI. It doesn't has any info about the
real length of
`val`. For backend, it has no other choice instead of `DINS`.
> > So the operation will be SImode, aka `INS` instruction for MIPS64.
> >
> > The problem is based on 2 fact/root cause:
> > 1. MIPS's `INS` instruction will be always to sign-extension, while `DINS`
> > won't
> > li $7, 0xff
> > li $8, 0
> > ins $8,$7,24,8 # set the 24-32 bits of $8 to 0xff.
> > The value of $8 will be 0xff ff ff ff ff 00 00 00.
>
> Bit that's wrong. (set (zero_extract:SI ...) should not affect
> bits outside of the indicated range.
>
In fact, it is how sign-extension arch work.
No matter wrong or right, the ISA was/is defined like this.
In fact, one MIPS 32 ABI, the same C code will generate the RTL like this,
and the 32bit object can still workable on 64bit CPU.
That's a smart (or brain-damaged) design.
> @findex zero_extract
> @item (zero_extract:@var{m} @var{loc} @var{size} @var{pos})
> Like @code{sign_extract} but refers to an unsigned or zero-extended
> bit-field. The same sequence of bits are extracted, but they
> are filled to an entire word with zeros instead of by sign-extension.
>
That's depending on the definition of `word` here.
For `(zero_extract:SI`, I think that the word is limit to the low 32bit of
hardware register.
Anyway, it won't break ISA without sign-extension by default.
Due to the nature of sign-extension ISA, if we don't sign-extension the
`int` variable, it will make something wrong.
To make it clear: the word `sign extension` here means:
the the value of 31bit will be copied to bits [32-63], and
the value of bits[0-30] won't be copied.
Here is the examples:
li $7, 0xff
li $8, 0x00 00 ff 00
ins $8,$7,16,8
^^
The value of $8 will be: 0x 00 00 00 00 00 ff ff 00
li $7, 0xff
li $8, 0x00 00 ff 00
ins $8,$7,24,8
^^
The value of $8 will be: 0x ff ff ff ff ff 00 ff 00
> Unlike @code{sign_extract}, this type of expressions can be lvalues
> in RTL; they may appear on the left side of an assignment, indicating
> insertion of a value into the specified bit-field.
> @end table
>
>
> > li $7, 0xff
> > li $8, 0
> > dins $8,$7,24,8 # set the 24-32 bits of $8 to 0xff.
> > The value of $8 will be 0x 00 00 00 00 ff 00 00 00.
>
> which isn't correct either.
>
It is not correct or not-correct: The ISA manual just state like this,
and the hardwares are working like this.
> If you look a few dumps further you'll see which instruction was
> recognized, I suspect the machine description is simply
Re: [PATCH v2] Store_bit_field_1: Use SUBREG instead of REG if possible
Richard Biener via Gcc-patches 于2023年7月19日周三 14:27写道：
>
> On Wed, 19 Jul 2023, YunQiang Su wrote:
>
> > PR #104914
> >
> > When work with
> > int val;
> > ((unsigned char*))[3] = *buf;
> > if (val > 0) ...
> > The RTX mode is obtained from REG instead of SUBREG, which make
> > D is used instead of . Thus something wrong happens
> > on sign-extend default architectures, like MIPS64.
> >
> > Let's use str_rtx and mode of str_rtx as the parameters for
> > store_integral_bit_field if:
> > modes of op0 and str_rtx are INT;
> > length of op0 is greater than str_rtx.
> >
> > This patch has been tested on aarch64-linux-gnu, x86_64-linux-gnu,
> > mips64el-linux-gnuabi64 without regression.
>
> I still think you are "fixing" this in the wrong place. The bugzilla
> audit trail points to combine and later notes an eventual expansion
> issue (but for another testcase/target).
>
> You have to explain in more detail on what is wrong with the initial
> RTL on mips.
>
In the first RTL file, aka xx.c.256r.expand, the zero_extract RTX is like
(insn 10 9 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
(const_int 8 [0x8])
(const_int 0 [0]))
(subreg:DI (reg:QI 202) 0)) "../xx.c":4:29 -1
(nil))
Not, all of the REG are in DImode. On MIPS64, it will expand to `DINS`
instructions.
While in fact here, we expect an SImode operation, due to `val` in C
code is `int`.
With my patch, the RTX will be like:
(insn 10 9 11 2 (set (zero_extract:SI (subreg:SI (reg/v:DI 200 [ val ]) 0)
(const_int 8 [0x8])
(const_int 0 [0]))
(subreg:SI (reg:QI 202) 0)) "xx.c":4:29 -1
(nil))
So the operation will be SImode, aka `INS` instruction for MIPS64.
The problem is based on 2 fact/root cause:
1. MIPS's `INS` instruction will be always to sign-extension, while `DINS` won't
li $7, 0xff
li $8, 0
ins $8,$7,24,8 # set the 24-32 bits of $8 to 0xff.
The value of $8 will be 0xff ff ff ff ff 00 00 00.
li $7, 0xff
li $8, 0
dins $8,$7,24,8 # set the 24-32 bits of $8 to 0xff.
The value of $8 will be 0x 00 00 00 00 ff 00 00 00.
2. Due to most of MIPS instructions work with 32bit value, aka instructions
without `d` as its first char (in fact with few exception), are sign-extension,
the MIPS backend just ignore `extendsidi2`, aka RTX
(insn 14 13 15 2 (set (reg/v:DI 200 [ val ])
(sign_extend:DI (subreg:SI (reg/v:DI 200 [ val ]) 0))) "xx.c":5:29 -1
(nil))
> Richard.
>
> > gcc/ChangeLog:
> > PR: 104914.
> > * expmed.cc(store_bit_field_1): Pass str_rtx and its mode
> > to store_integral_bit_field if the length of op0 is greater
> > than str_rtx.
> >
> > gcc/testsuite/ChangeLog:
> > PR: 104914.
> > * gcc.target/mips/pr104914.c: New testcase.
> > ---
> > gcc/expmed.cc| 20 +---
> > gcc/testsuite/gcc.target/mips/pr104914.c | 17 +
> > 2 files changed, 34 insertions(+), 3 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.target/mips/pr104914.c
> >
> > diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> > index fbd4ce2d42f..5531c19e891 100644
> > --- a/gcc/expmed.cc
> > +++ b/gcc/expmed.cc
> > @@ -850,6 +850,7 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize,
> > poly_uint64 bitnum,
> > since that case is valid for any mode. The following cases are only
> > valid for integral modes. */
> >opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (op0));
> > + opt_scalar_int_mode str_mode = int_mode_for_mode (GET_MODE (str_rtx));
> >scalar_int_mode imode;
> >if (!op0_mode.exists () || imode != GET_MODE (op0))
> > {
> > @@ -881,9 +882,22 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize,
> > poly_uint64 bitnum,
> > op0 = gen_lowpart (op0_mode.require (), op0);
> > }
> >
> > - return store_integral_bit_field (op0, op0_mode, ibitsize, ibitnum,
> > -bitregion_start, bitregion_end,
> > -fieldmode, value, reverse, fallback_p);
> > + /* If MODEs of str_rtx and op0 are INT, and the length of op0 is greater
> > than
> > + str_rtx, it means that str_rtx has a shorter SUBREG: int32 on 64
> > mach/ABI
> > + is an example. For this case, we should use the mode of SUBREG,
> > otherwise
> > + bad code will generate for sign-extension ports, like MIPS. */
> > + bool use_str_mode = false;
> > + if (GET_MODE_CLASS (GET_MODE (str_rtx)) == MODE_INT
> > + && GET_MODE_CLASS (GET_MODE (op0)) == MODE_INT
> > + && known_gt (GET_MODE_SIZE (GET_MODE (op0)),
> > +GET_MODE_SIZE (GET_MODE (str_rtx
> > +use_str_mode = true;
> > +
> > + return store_integral_bit_field (use_str_mode ? str_rtx : op0,
> > +use_str_mode ? str_mode : op0_mode,
> > +ibitsize, ibitnum, bitregion_start,
> > +bitregion_end,
Re: [PATCH] MIPS: Adjust mips16e2 related tests for ifcvt costing changes
Jie Mei 于2023年7月4日周二 17:52写道：
>
> A mips16e2 related test fails after the ifcvt change. The mips16e2
> addition also causes a test for unrelated module to fail.
>
> This patch adjusts branch costs when running the two affected tests.
>
> These tests should not require the -mbranch-cost option, and
> this issue needs to be addressed.
>
Wish it soon.
> gcc/testsuite/ChangeLog:
>
> * gcc.target/mips/mips16e2-cmov.c: Adjust branch cost to
> encourage if-conversion.
> * gcc.target/mips/movcc-3.c: Same as above.
> ---
> gcc/testsuite/gcc.target/mips/mips16e2-cmov.c | 2 +-
> gcc/testsuite/gcc.target/mips/movcc-3.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/testsuite/gcc.target/mips/mips16e2-cmov.c
> b/gcc/testsuite/gcc.target/mips/mips16e2-cmov.c
> index 6e9dd82ebf3..129ea23b65b 100644
> --- a/gcc/testsuite/gcc.target/mips/mips16e2-cmov.c
> +++ b/gcc/testsuite/gcc.target/mips/mips16e2-cmov.c
> @@ -1,4 +1,4 @@
> -/* { dg-options "-mno-abicalls -mgpopt -G8 -mabi=32 -mips16 -mmips16e2" } */
> +/* { dg-options "-mno-abicalls -mgpopt -G8 -mabi=32 -mips16 -mmips16e2
> -mbranch-cost=2" } */
> /* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
>
> /* Test MOVN. */
> diff --git a/gcc/testsuite/gcc.target/mips/movcc-3.c
> b/gcc/testsuite/gcc.target/mips/movcc-3.c
> index 80d44098a3f..569a00423c1 100644
> --- a/gcc/testsuite/gcc.target/mips/movcc-3.c
> +++ b/gcc/testsuite/gcc.target/mips/movcc-3.c
> @@ -1,5 +1,5 @@
> /* { dg-do compile } */
> -/* { dg-options "(HAS_MOVN) -mhard-float -mbranch-cost=2" } */
> +/* { dg-options "(HAS_MOVN) -mhard-float -mbranch-cost=3" } */
> /* { dg-skip-if "code quality test" { *-*-* } { "-O0" "-Os" } { "" } } */
> /* { dg-final { scan-assembler "\tmovt\t" } } */
> /* { dg-final { scan-assembler "\tmovf\t" } } */
> --
> 2.40.1
--
YunQiang Su
Re: [PATCH v2] mips: Fix overaligned function arguments [PR109435]
YunQiang Su 于2023年6月29日周四 14:04写道：
>
> Jovan Dmitrovic 于2023年6月27日周二 16:54写道：
> >
> > Hi,
> > I am sending a revised patch, now with different tests for N64/N32 and O32
> > ABIs.
> > For the O32 ABI, I've skipped the -O0 and -Os pipelines, considering there
> > is a
> > difference between exact offsets for store instructions (the registers used
> > remain
> > the same).
> >
> > Skipping -flto isn't really necessary, so I've removed that part.
> >
> > I've fixed the Changelog, hopefully I've corrected the mistakes I made.
> >
>
> Looks good.
> I will submit this patch with some format improvement.
>
Ohh, my fault: the `-flto` option should always be skipped, when run test.
And you skipped -O0 test on O32, while this bug effects O0 only, it
should not be expected.
The below is my modification to your patch.
Is it OK for you?
--- xx.patch 2023-06-29 14:32:59.805474033 +0800
+++ build/0001-mips-Fix-overaligned-function-arguments-PR109435.patch
2023-06-29 18:01:19.245478275 +0800
@@ -1,4 +1,4 @@
-From 05e4ff4d2fbb91ea8040fb10d8d6a130ad24bba7 Mon Sep 17 00:00:00 2001
+From 7b5af22bb7c8fadce27e94c37c96101a06acd286 Mon Sep 17 00:00:00 2001
From: Jovan Dmitrovic
Date: Mon, 26 Jun 2023 17:00:20 +0200
Subject: [PATCH] mips: Fix overaligned function arguments [PR109435]
@@ -16,12 +16,13 @@
2023-06-27 Jovan Dmitrović
gcc/ChangeLog:
-PR target/109435
+
+ PR target/109435
* config/mips/mips.cc (mips_function_arg_alignment): Returns
-the alignment of function argument. In case of typedef type,
-it returns the aligment of the aliased type.
+ the alignment of function argument. In case of typedef type,
+ it returns the aligment of the aliased type.
(mips_function_arg_boundary): Relocated calculation of the
-aligment of function arguments.
+ aligment of function arguments.
gcc/testsuite/ChangeLog:
@@ -29,9 +30,9 @@
* gcc.target/mips/align-1-o32.c: New test.
---
gcc/config/mips/mips.cc | 19 ++-
- gcc/testsuite/gcc.target/mips/align-1-n64.c | 19 +++
+ gcc/testsuite/gcc.target/mips/align-1-n64.c | 20
gcc/testsuite/gcc.target/mips/align-1-o32.c | 20
- 3 files changed, 57 insertions(+), 1 deletion(-)
+ 3 files changed, 58 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/mips/align-1-n64.c
create mode 100644 gcc/testsuite/gcc.target/mips/align-1-o32.c
@@ -75,14 +76,15 @@
if (alignment > STACK_BOUNDARY)
diff --git a/gcc/testsuite/gcc.target/mips/align-1-n64.c
b/gcc/testsuite/gcc.target/mips/align-1-n64.c
new file mode 100644
-index 000..46e718d548d
+index 000..3ede539c3a4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/align-1-n64.c
-@@ -0,0 +1,19 @@
+@@ -0,0 +1,20 @@
+/* Check that typedef alignment does not affect passing of function
+ parameters for N64/N32 ABIs. */
+/* { dg-do compile { target { "mips*-*-*" } } } */
+/* { dg-options "-mabi=64" } */
++/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */
+
+typedef struct ui8
+{
@@ -100,7 +102,7 @@
+/* { dg-final { scan-assembler "\tsd\t\\\$7,16\\(\\\$\[0-9\]\\)" } } */
diff --git a/gcc/testsuite/gcc.target/mips/align-1-o32.c
b/gcc/testsuite/gcc.target/mips/align-1-o32.c
new file mode 100644
-index 000..a548632b7f6
+index 000..e043d6a3eca
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/align-1-o32.c
@@ -0,0 +1,20 @@
@@ -108,7 +110,7 @@
+ parameters for O32 ABI. */
+/* { dg-do compile { target { "mips*-*-*" } } } */
+/* { dg-options "-mabi=32" } */
-+/* { dg-skip-if "" { *-*-* } { "-O0" "-Os" } { "" } } */
++/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */
+
+typedef struct ui8
+{
@@ -121,10 +123,9 @@
+ return a.v[0];
+}
+
-+/* { dg-final { scan-assembler "\tsw\t\\\$5,100\\(\\\$sp\\)" } } */
-+/* { dg-final { scan-assembler "\tsw\t\\\$6,104\\(\\\$sp\\)" } } */
-+/* { dg-final { scan-assembler "\tsw\t\\\$7,108\\(\\\$sp\\)" } } */
++/* { dg-final { scan-assembler "\tsw\t\\\$5,1\\d\\d\\(\\\$(sp|fp)\\)" } } */
++/* { dg-final { scan-assembler "\tsw\t\\\$6,1\\d\\d\\(\\\$(sp|fp)\\)" } } */
++/* { dg-final { scan-assembler "\tsw\t\\\$7,1\\d\\d\\(\\\$(sp|fp)\\)" } } */
--
-2.34.1
-
+2.30.2
> > Regards,
> > Jovan
>
>
>
> --
> YunQiang Su
--
YunQiang Su
Re: [PATCH v2] mips: Fix overaligned function arguments [PR109435]
Jovan Dmitrovic 于2023年6月27日周二 16:54写道： > > Hi, > I am sending a revised patch, now with different tests for N64/N32 and O32 > ABIs. > For the O32 ABI, I've skipped the -O0 and -Os pipelines, considering there is > a > difference between exact offsets for store instructions (the registers used > remain > the same). > > Skipping -flto isn't really necessary, so I've removed that part. > > I've fixed the Changelog, hopefully I've corrected the mistakes I made. > Looks good. I will submit this patch with some format improvement. > Regards, > Jovan -- YunQiang Su
Re: [PATCH] mips: Fix overaligned function arguments [PR109435]
Jovan Dmitrovic 于2023年6月7日周三 18:29写道：
>
> I see what you mean now, so I've made adjustment in order for testcase to work
> on assembly. Following is the updated patch.
>
> Regards,
> Jovan
>
> From 2744357b5232c61bf1f780c4915d47b19d71f993 Mon Sep 17 00:00:00 2001
> From: Jovan Dmitrovic
> Date: Fri, 19 May 2023 12:36:55 +0200
> Subject: [PATCH] mips: Fix overaligned function arguments [PR109435]
>
> This patch changes alignment for typedef types when passed as
> arguments, making the alignment equal to the alignment of
> original (aliased) types.
>
> This change makes it impossible for a typedef type to have
> alignment that is less than its size.
>
> Signed-off-by: Jovan Dmitrovic
>
> gcc/ChangeLog:
> PR target/109435
> * config/mips/mips.cc (mips_function_arg_alignment): Returns
> the alignment of function argument. In case of typedef type,
> it returns the aligment of the aliased type.
> (mips_function_arg_boundary): Relocated calculation of the
> aligment of function arguments.
>
Please refer
https://gcc.gnu.org/contribute.html
about how to work with the ChangeLog.
> gcc/testsuite/ChangeLog:
> PR target/109435
> * gcc.target/mips/align-1.c: New test.
> ---
> gcc/config/mips/mips.cc | 19 -
> gcc/testsuite/gcc.target/mips/align-1.c | 38 +
> 2 files changed, 56 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/mips/align-1.c
>
> diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> index c1d1691306e..20ba35f754c 100644
> --- a/gcc/config/mips/mips.cc
> +++ b/gcc/config/mips/mips.cc
> @@ -6190,6 +6190,23 @@ mips_arg_partial_bytes (cumulative_args_t cum, const
> function_arg_info )
>return info.stack_words > 0 ? info.reg_words * UNITS_PER_WORD : 0;
> }
>
> +/* Given MODE and TYPE of a function argument, return the alignment in
> + bits.
> + In case of typedef, alignment of its original type is
> + used. */
> +
> +static unsigned int
> +mips_function_arg_alignment (machine_mode mode, const_tree type)
> +{
> + if (!type)
> +return GET_MODE_ALIGNMENT (mode);
> +
> + if (is_typedef_decl (TYPE_NAME (type)))
> +type = DECL_ORIGINAL_TYPE (TYPE_NAME (type));
> +
> + return TYPE_ALIGN (type);
> +}
> +
> /* Implement TARGET_FUNCTION_ARG_BOUNDARY. Every parameter gets at
> least PARM_BOUNDARY bits of alignment, but will be given anything up
> to STACK_BOUNDARY bits if the type requires it. */
> @@ -6198,8 +6215,8 @@ static unsigned int
> mips_function_arg_boundary (machine_mode mode, const_tree type)
> {
>unsigned int alignment;
> + alignment = mips_function_arg_alignment (mode, type);
>
> - alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
>if (alignment < PARM_BOUNDARY)
> alignment = PARM_BOUNDARY;
>if (alignment > STACK_BOUNDARY)
> diff --git a/gcc/testsuite/gcc.target/mips/align-1.c
> b/gcc/testsuite/gcc.target/mips/align-1.c
> new file mode 100644
> index 000..5c639bee274
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/align-1.c
> @@ -0,0 +1,38 @@
> +/* Check that typedef alignment does not affect passing of function
> + parameters. */
> +/* { dg-do compile { target { "mips*-*-linux*" } } } */
mips* may be OK, since this test looks reasonable for bare metal platforms.
> +/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */
Does `-flto` required here?
> +
> +#include
> +
> +typedef struct ui8
> +{
> + unsigned v[8];
> +} uint8 __attribute__ ((aligned(64)));
> +
> +unsigned
> +callee (int x, uint8 a)
> +{
> + return a.v[0];
> +}
> +
> +uint8
> +identity (uint8 in)
> +{
> + return in;
> +}
> +
> +int
> +main (void)
> +{
> + uint8 vec = {{1, 2, 3, 4, 5, 6, 7, 8}};
> + uint8 temp = identity (vec);
> + unsigned temp2 = callee (1, identity (vec));
> + assert (callee (1, temp) == 1);
> + assert (temp2 == 1);
> + return 0;
> +}
> +
> +/* { dg-final { scan-assembler "\tsd\t\\\$5,0\\(\\\$\[0-9\]\\)" } } */
> +/* { dg-final { scan-assembler "\tsd\t\\\$6,8\\(\\\$\[0-9\]\\)" } } */
> +/* { dg-final { scan-assembler "\tsd\t\\\$7,16\\(\\\$\[0-9\]\\)" } } */
I guess, this test may fail for mips32 targets?
Maybe we can add 2 tests: one for O32, and one for N32/N64.
Add `-mabi=32`/`-mabi=n32` option into `dg-do compile` line.
> --
> 2.34.1
>
>
>
>
> --
> YunQiang Su
--
YunQiang Su
Re: [PATCH] mips: Fix overaligned function arguments [PR109435]
Jovan Dmitrovic 于2023年6月6日周二 18:29写道：
>
> I suppose that it is possible to check assembly.
>
Great.
> Following is part of diff before and after my patch:
>
> 29,32c29,32
> < sd $6,0($2)
> < sd $7,8($2)
> < sd $8,16($2)
> < sd $9,24($2)
> ---
> > sd $5,0($2)
> > sd $6,8($2)
> > sd $7,16($2)
> > sd $8,24($2)
> 63,66c63,66
> < sd $6,0($2)
> < sd $7,8($2)
> < sd $8,16($2)
> < sd $9,24($2)
> ---
> > sd $5,0($2)
> > sd $6,8($2)
> > sd $7,16($2)
> > sd $8,24($2)
> 138,141c138,141
> < ld $6,64($16)
> < ld $7,72($16)
> < ld $8,80($16)
> < ld $9,88($16)
> ---
> > ld $5,64($16)
> > ld $6,72($16)
> > ld $7,80($16)
> > ld $8,88($16)
> 148,151c148,151
> < ld $6,64($16)
> < ld $7,72($16)
> < ld $8,80($16)
> < ld $9,88($16)
> ---
> > ld $5,64($16)
> > ld $6,72($16)
> > ld $7,80($16)
> > ld $8,88($16)
> 167,170c167,170
> < ld $6,0($16)
> < ld $7,8($16)
> < ld $8,16($16)
> < ld $9,24($16)
> ---
> > ld $5,0($16)
> > ld $6,8($16)
> > ld $7,16($16)
> > ld $8,24($16)
>
> What my patch effectively does it rearranges the data in
> registers when invoking a function. I don't know whether
> writing this testcase as an assembly check would make sense,
> because that would make the testcase much less readable.
I prefer an assembly check, because the test can be used even
for cross building.
It is not required, I guess.
>
> From: YunQiang Su
> Sent: Wednesday, May 31, 2023 12:05 PM
> To: Jovan Dmitrovic
> Cc: gcc-patches@gcc.gnu.org; Djordje Todorovic
> Subject: Re: [PATCH] mips: Fix overaligned function arguments [PR109435]
>
> Jovan Dmitrovic 于2023年5月29日周一 19:00写道：
> >
> > This patch changes alignment for typedef types when passed as
> > arguments, making the alignment equal to the alignment of
> > original (aliased) types.
> >
> > This change makes it impossible for a typedef type to have
> > alignment that is less than its size.
> >
> > Signed-off-by: Jovan Dmitrovic
> >
> > gcc/ChangeLog:
> > PR target/109435
> > * config/mips/mips.cc (mips_function_arg_alignment): Returns
> > the alignment of function argument. In case of typedef type,
> > it returns the aligment of the aliased type.
> > (mips_function_arg_boundary): Relocated calculation of the
> > aligment of function arguments.
> >
> > gcc/testsuite/ChangeLog:
> > PR target/109435
> > * gcc.target/mips/align-1.c: New test.
> > ---
> > gcc/config/mips/mips.cc | 18 +-
> > gcc/testsuite/gcc.target/mips/align-1.c | 33 +
> > 2 files changed, 50 insertions(+), 1 deletion(-)
> > create mode 100644 gcc/testsuite/gcc.target/mips/align-1.c
> >
> > diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> > index ca822758b41..2019b7cd7d9 100644
> > --- a/gcc/config/mips/mips.cc
> > +++ b/gcc/config/mips/mips.cc
> > @@ -6190,6 +6190,22 @@ mips_arg_partial_bytes (cumulative_args_t cum, const
> > function_arg_info )
> >return info.stack_words > 0 ? info.reg_words * UNITS_PER_WORD : 0;
> > }
> >
> > +/* Given MODE and TYPE of a function argument, return the alignment in
> > + bits. In case of typedef, alignment of its original type is
> > + used. */
> > +
> > +static unsigned int
> > +mips_function_arg_alignment (machine_mode mode, const_tree type)
> > +{
> > + if (!type)
> > +return GET_MODE_ALIGNMENT (mode);
> > +
> > + if (is_typedef_decl (TYPE_NAME (type)))
> > +type = DECL_ORIGINAL_TYPE (TYPE_NAME (type));
> > +
> > + return TYPE_ALIGN (type);
> > +}
> > +
> > /* Implement TARGET_FUNCTION_ARG_BOUNDARY. Every parameter gets at
> > least PARM_BOUNDARY bits of alignment, but will be given anything up
> > to STACK_BOUNDARY bits if the type requires it. */
> > @@ -6198,8 +6214,8 @@ static unsigned int
> > mips_function_arg_boundary (machine_mode mode, const_tree type)
> > {
> >unsigned int alignment;
> > + alignment = mips_function_arg_alignment (mode, type);
> >
> > - alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
> >if (alignment < PARM_BOUNDARY)
> > alignment = PARM_BOUNDARY;
> >if (alignment > STACK_BOUNDARY)
> > diff --git a/gcc/testsuite/gcc.target/mips/align-1.c
> > b/gcc/testsuite/gcc.target/mips/align-1.c
> > new file mode 100644
> > index 000..816751b8099
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/mips/align-1.c
> > @@ -0,0 +1,33 @@
> > +/* Check that typedef alignment does not affect passing of function
> > + parameters. */
> > +/* { dg-do run { target { "mips*-*-linux*" } } } */
>
> Is it
Re: [PATCH v2] MIPS16: Implement `code_readable` function attribute.
Jie Mei 于2023年5月19日周五 16:07写道：
>
> From: Simon Dardis
>
> Support for __attribute__ ((code_readable)). Takes up to one argument of
> "yes", "no", "pcrel". This will change the code readability setting for just
> that function. If no argument is supplied, then the setting is 'yes'.
>
> gcc/ChangeLog:
>
> * config/mips/mips.cc (enum mips_code_readable_setting):New enmu.
> (mips_handle_code_readable_attr):New static function.
> (mips_get_code_readable_attr):New static enum function.
> (mips_set_current_function):Set the code_readable mode.
> (mips_option_override):Same as above.
> * doc/extend.texi:Document code_readable.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/mips/code-readable-attr-1.c: New test.
> * gcc.target/mips/code-readable-attr-2.c: New test.
> * gcc.target/mips/code-readable-attr-3.c: New test.
> * gcc.target/mips/code-readable-attr-4.c: New test.
> * gcc.target/mips/code-readable-attr-5.c: New test.
> ---
> gcc/config/mips/mips.cc | 97 ++-
> gcc/doc/extend.texi | 17
> .../gcc.target/mips/code-readable-attr-1.c| 51 ++
> .../gcc.target/mips/code-readable-attr-2.c| 49 ++
> .../gcc.target/mips/code-readable-attr-3.c| 50 ++
> .../gcc.target/mips/code-readable-attr-4.c| 51 ++
> .../gcc.target/mips/code-readable-attr-5.c| 5 +
> 7 files changed, 319 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/mips/code-readable-attr-1.c
> create mode 100644 gcc/testsuite/gcc.target/mips/code-readable-attr-2.c
> create mode 100644 gcc/testsuite/gcc.target/mips/code-readable-attr-3.c
> create mode 100644 gcc/testsuite/gcc.target/mips/code-readable-attr-4.c
> create mode 100644 gcc/testsuite/gcc.target/mips/code-readable-attr-5.c
>
> diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> index ca822758b41..97f45e67529 100644
> --- a/gcc/config/mips/mips.cc
> +++ b/gcc/config/mips/mips.cc
> @@ -498,6 +498,9 @@ static int mips_base_target_flags;
> /* The default compression mode. */
> unsigned int mips_base_compression_flags;
>
> +/* The default code readable setting. */
> +enum mips_code_readable_setting mips_base_code_readable;
> +
> /* The ambient values of other global variables. */
> static int mips_base_schedule_insns; /* flag_schedule_insns */
> static int mips_base_reorder_blocks_and_partition; /* flag_reorder... */
> @@ -602,6 +605,7 @@ const enum reg_class
> mips_regno_to_class[FIRST_PSEUDO_REGISTER] = {
>ALL_REGS,ALL_REGS, ALL_REGS, ALL_REGS
> };
>
> +static tree mips_handle_code_readable_attr (tree *, tree, tree, int, bool *);
> static tree mips_handle_interrupt_attr (tree *, tree, tree, int, bool *);
> static tree mips_handle_use_shadow_register_set_attr (tree *, tree, tree,
> int,
> bool *);
> @@ -623,6 +627,8 @@ static const struct attribute_spec mips_attribute_table[]
> = {
>{ "micromips", 0, 0, true, false, false, false, NULL, NULL },
>{ "nomicromips", 0, 0, true, false, false, false, NULL, NULL },
>{ "nocompression", 0, 0, true, false, false, false, NULL, NULL },
> + { "code_readable", 0, 1, true, false, false, false,
> +mips_handle_code_readable_attr, NULL },
>/* Allow functions to be specified as interrupt handlers */
>{ "interrupt", 0, 1, false, true, true, false,
> mips_handle_interrupt_attr,
> NULL },
> @@ -1310,6 +1316,81 @@ mips_use_debug_exception_return_p (tree type)
>TYPE_ATTRIBUTES (type)) != NULL;
> }
>
> +
> +/* Verify the arguments to a code_readable attribute. */
> +
> +static tree
> +mips_handle_code_readable_attr (tree *node ATTRIBUTE_UNUSED, tree name,
> + tree args, int flags ATTRIBUTE_UNUSED,
> + bool *no_add_attrs)
> +{
> + if (!is_attribute_p ("code_readable", name) || args == NULL)
> +return NULL_TREE;
> +
> + if (TREE_CODE (TREE_VALUE (args)) != STRING_CST)
> +{
> + warning (OPT_Wattributes,
> + "%qE attribute requires a string argument", name);
> + *no_add_attrs = true;
> +}
> + else if (strcmp (TREE_STRING_POINTER (TREE_VALUE (args)), "no") != 0
> + && strcmp (TREE_STRING_POINTER (TREE_VALUE (args)), "pcrel") != 0
> + && strcmp (TREE_STRING_POINTER (TREE_VALUE (args)), "yes") != 0)
> +{
> + warning (OPT_Wattributes,
> + "argument to %qE attribute is neither no, pcrel nor yes",
> name);
> + *no_add_attrs = true;
> +}
> +
> + return NULL_TREE;
> +}
> +
> +/* Determine the code_readable setting for a function if it has one. Set
> + *valid to true if we have a properly formed argument and
> + return the result. If there's no argument, return GCC's default.
contrib/check_GNU_style.sh complains that one more
Re: [PATCH] mips: Fix overaligned function arguments [PR109435]
Jovan Dmitrovic 于2023年5月29日周一 19:00写道：
>
> This patch changes alignment for typedef types when passed as
> arguments, making the alignment equal to the alignment of
> original (aliased) types.
>
> This change makes it impossible for a typedef type to have
> alignment that is less than its size.
>
> Signed-off-by: Jovan Dmitrovic
>
> gcc/ChangeLog:
> PR target/109435
> * config/mips/mips.cc (mips_function_arg_alignment): Returns
> the alignment of function argument. In case of typedef type,
> it returns the aligment of the aliased type.
> (mips_function_arg_boundary): Relocated calculation of the
> aligment of function arguments.
>
> gcc/testsuite/ChangeLog:
> PR target/109435
> * gcc.target/mips/align-1.c: New test.
> ---
> gcc/config/mips/mips.cc | 18 +-
> gcc/testsuite/gcc.target/mips/align-1.c | 33 +
> 2 files changed, 50 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/mips/align-1.c
>
> diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> index ca822758b41..2019b7cd7d9 100644
> --- a/gcc/config/mips/mips.cc
> +++ b/gcc/config/mips/mips.cc
> @@ -6190,6 +6190,22 @@ mips_arg_partial_bytes (cumulative_args_t cum, const
> function_arg_info )
>return info.stack_words > 0 ? info.reg_words * UNITS_PER_WORD : 0;
> }
>
> +/* Given MODE and TYPE of a function argument, return the alignment in
> + bits. In case of typedef, alignment of its original type is
> + used. */
> +
> +static unsigned int
> +mips_function_arg_alignment (machine_mode mode, const_tree type)
> +{
> + if (!type)
> +return GET_MODE_ALIGNMENT (mode);
> +
> + if (is_typedef_decl (TYPE_NAME (type)))
> +type = DECL_ORIGINAL_TYPE (TYPE_NAME (type));
> +
> + return TYPE_ALIGN (type);
> +}
> +
> /* Implement TARGET_FUNCTION_ARG_BOUNDARY. Every parameter gets at
> least PARM_BOUNDARY bits of alignment, but will be given anything up
> to STACK_BOUNDARY bits if the type requires it. */
> @@ -6198,8 +6214,8 @@ static unsigned int
> mips_function_arg_boundary (machine_mode mode, const_tree type)
> {
>unsigned int alignment;
> + alignment = mips_function_arg_alignment (mode, type);
>
> - alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
>if (alignment < PARM_BOUNDARY)
> alignment = PARM_BOUNDARY;
>if (alignment > STACK_BOUNDARY)
> diff --git a/gcc/testsuite/gcc.target/mips/align-1.c
> b/gcc/testsuite/gcc.target/mips/align-1.c
> new file mode 100644
> index 000..816751b8099
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/align-1.c
> @@ -0,0 +1,33 @@
> +/* Check that typedef alignment does not affect passing of function
> + parameters. */
> +/* { dg-do run { target { "mips*-*-linux*" } } } */
Is it possible to check the result with something like
scan-assembler
scan-assembler-not
instead of real running?
> +
> +#include
> +
> +typedef struct ui8
> +{
> + unsigned v[8];
> +} uint8 __attribute__ ((aligned(64)));
> +
> +unsigned
> +callee (int x, uint8 a)
> +{
> + return a.v[0];
> +}
> +
> +uint8
> +identity (uint8 in)
> +{
> + return in;
> +}
> +
> +int
> +main (void)
> +{
> + uint8 vec = {{1, 2, 3, 4, 5, 6, 7, 8}};
> + uint8 temp = identity (vec);
> + unsigned temp2 = callee (1, identity (vec));
> + assert (callee (1, temp) == 1);
> + assert (temp2 == 1);
> + return 0;
> +}
> --
> 2.34.1
>
--
YunQiang Su
Re: [PATCH] MIPS: fix building on multiarch platform
Xi Ruoyao via Gcc-patches 于2022年9月21日周三 23:09写道：
>
> On Wed, 2022-09-21 at 11:31 +, YunQiang Su wrote:
> > On platforms that support multiarch, such as Debian,
> > the filesystem hierarchy doesn't fellow the old Irix style:
> > lib & lib/ for native
> > lib64 for N64 on N32/O32 systems
> > lib32 for N32 on N64/O32 systems
> > libo32 for O32 on N64/N32 systems
> >
> > Thus we cannot
> > #define STANDARD_STARTFILE_PREFIX_1
> > #define STANDARD_STARTFILE_PREFIX_2
> > on N32 or N64 systems, else collect2 won't look for libraries
> > on /lib/.
> >
> > gcc/ChangeLog:
> > * configure.ac: AC_DEFINE(ENABLE_MULTIARCH, 1)
> > * configure: Regenerated.
> > * config.in: Regenerated.
> > * config/mips/mips.h: don't define STANDARD_STARTFILE_PREFIX_1
> > if ENABLE_MULTIARCH is defined.
> > * config/mips/t-linux64: define correct multiarch path when
> > multiarch is enabled.
> > ---
> > gcc/config.in | 6 ++
> > gcc/config/mips/mips.h| 2 ++
> > gcc/config/mips/t-linux64 | 21 -
> > gcc/configure | 4
> > gcc/configure.ac | 3 +++
> > 5 files changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/gcc/config.in b/gcc/config.in
> > index 6ac17be189e..b2ce6361327 100644
> > --- a/gcc/config.in
> > +++ b/gcc/config.in
> > @@ -2312,6 +2312,12 @@
> > #endif
> >
> >
> > +/* Specify if mutliarch is enabled. */
> > +#ifndef USED_FOR_TARGET
> > +#undef ENABLE_MULTIARCH
> > +#endif
> > +
> > +
> > /* The size of `dev_t', as computed by sizeof. */
> > #ifndef USED_FOR_TARGET
> > #undef SIZEOF_DEV_T
> > diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
> > index 74b6e11aabb..fe7f5b274b9 100644
> > --- a/gcc/config/mips/mips.h
> > +++ b/gcc/config/mips/mips.h
> > @@ -3427,6 +3427,7 @@ struct GTY(()) machine_function {
> >
> > /* If we are *not* using multilibs and the default ABI is not ABI_32
> > we
> > need to change these from /lib and /usr/lib. */
> > +#ifndef ENABLE_MULTIARCH
> > #if MIPS_ABI_DEFAULT == ABI_N32
> > #define STANDARD_STARTFILE_PREFIX_1 "/lib32/"
> > #define STANDARD_STARTFILE_PREFIX_2 "/usr/lib32/"
> > @@ -3434,6 +3435,7 @@ struct GTY(()) machine_function {
> > #define STANDARD_STARTFILE_PREFIX_1 "/lib64/"
> > #define STANDARD_STARTFILE_PREFIX_2 "/usr/lib64/"
> > #endif
> > +#endif
>
> Should we just remove STANDARD_STARTFILE_PREFIX_{1,2} unconditionally?
> I just took a look and the only Linux ports using these macros are MIPS
> and LoongArch (borrowed these macros from MIPS, I guess). On a non-
> multilib distro /usr/lib is likely used, and on multilib distros the
> macros are not used anyway.
>
Yes. As Maciej pointed, the old mips style for MIPS filesystem
hierarchy is quite quirk.
This is from Irix.
It does make lots of trouble for distribution makers to support
multilib for mips64,
since
On non-multilib distributions, like OpenWrt for mips64, lib is used,
and lib64 is a symlink to lib.
While on multilib-enabled ports, like Fedora, the old Irix style is used.
So, we should keep the code.
> > /* Load store bonding is not supported by micromips and fix_24k. The
> > performance can be degraded for those targets. Hence, do not bond for
> > diff --git a/gcc/config/mips/t-linux64 b/gcc/config/mips/t-linux64
> > index 2fdd8e00407..37d176ea309 100644
> > --- a/gcc/config/mips/t-linux64
> > +++ b/gcc/config/mips/t-linux64
> > @@ -20,7 +20,26 @@ MULTILIB_OPTIONS = mabi=n32/mabi=32/mabi=64
> > MULTILIB_DIRNAMES = n32 32 64
> > MIPS_EL = $(if $(filter %el, $(firstword $(subst -, ,$(target,el)
> > MIPS_SOFT = $(if $(strip $(filter MASK_SOFT_FLOAT_ABI,
> > $(target_cpu_default)) $(filter soft, $(with_float))),soft)
> > -MULTILIB_OSDIRNAMES = \
> > +ifeq (yes,$(enable_multiarch))
> > + ifneq (,$(findstring gnuabi64,$(target)))
> > +MULTILIB_OSDIRNAMES = \
> > + ../lib32$(call
> > if_multiarch,:mips64$(MIPS_EL)-linux-gnuabin32$(MIPS_SOFT)) \
> > + ../libo32$(call if_multiarch,:mips$(MIPS_EL)-linux-gnu$(MIPS_SOFT))
> > \
> > + ../lib$(call
> > if_multiarch,:mips64$(MIPS_EL)-linux-gnuabi64$(MIPS_SOFT))
> > + else ifneq (,$(findstring gnuabin32,$(target)))
> > +MULTILIB_OSDIRNAMES = \
> > + ../lib$(call
> > if_multiarch,:mips64$(MIPS_EL)-linux-gnuabin32$(MIPS_SOFT)) \
> > + ../libo32$(call if_multiarch,:mips$(MIPS_EL)-linux-gnu$(MIPS_SOFT))
> > \
> > + ../lib64$(call
> > if_multiarch,:mips64$(MIPS_EL)-linux-gnuabi64$(MIPS_SOFT))
> > + else
> > +MULTILIB_OSDIRNAMES = \
> > + ../lib32$(call
> > if_multiarch,:mips64$(MIPS_EL)-linux-gnuabin32$(MIPS_SOFT)) \
> > + ../lib$(call if_multiarch,:mips$(MIPS_EL)-linux-gnu$(MIPS_SOFT)) \
> > + ../lib64$(call
> > if_multiarch,:mips64$(MIPS_EL)-linux-gnuabi64$(MIPS_SOFT))
> > + endif
> > +else
> > + MULTILIB_OSDIRNAMES = \
> > ../lib32$(call
> >
Re: [PATCH] skip testing time before epoch on mips
Jonathan Wakely via Gcc-patches 于2021年3月8日周一 下午6:57写道：
>
> On 08/03/21 14:35 +0800, Chen Li wrote:
> >
> >When execute libstdc++ testcases on mips, I notice that last_write_time
> >alawys failed, and the failed VERIFY is "VERIFY(
> >approx_equal(last_write_time(f.path), time) );" where testing time before
> >than epoch.
> >
> >Below is the minimal case:
> >
> >```
> >// gcc a.c
> >int main()
> >{
> >struct timespec times[2] = {{1, UTIME_OMIT}, {-1201, 98500}};
> >utimensat(AT_FDCWD, "test", times, 0);
> >
> >}
> >```
> >
> >$ touch test && gcc a.c && ./a.out && stat test
> > File: test
> > Size: 0 Blocks: 0 IO Block: 4096 regular empty
> > file
> >Device: 805h/2053d Inode: 1056841 Links: 1
> >Access: (0644/-rw-r--r--) Uid: ( 1000/ deepin) Gid: ( 1000/ deepin)
> >Access: 2021-03-08 13:52:55.966354501 +0800
> >Modify: 2106-02-07 14:08:15.98500 +0800
> >Change: 2021-03-08 13:52:56.907782193 +0800
> > Birth: -
> >
> >Undoubtedly, mtime's type is unsigned somewhere on mips.
> >
> >After debuging kernel, it turns out that mtime is always -1201 in
> >ext4_setattr, cp_new_stat, newlstat and etc, so the problem should not
> >occur in kernel space.
> >
> >go back to user space via copy_to_user, I finally found mips used
> >"unsigned int st_mtime_sec;" in struct kernel_stat, which is used to
> >receive -1201 from kernel.
>
> I can't reproduce this on a mips64 machine:
>
> jwakely@erpro8-fsf2:~$ uname -a
> Linux erpro8-fsf2 4.1.4 #1 SMP PREEMPT Mon Aug 3 14:22:54 PDT 2015 mips64
> GNU/Linux
> jwakely@erpro8-fsf2:~$ apt list libc6
> Listing... Done
> libc6/oldstable,now 2.19-18+deb8u10 mipsel [installed]
it is mips32, not mips64. Here mips64 we mean userland, not the kernel.
> jwakely@erpro8-fsf2:~$ touch test && ./a.out && TZ= stat test
>File: ‘test’
>Size: 0 Blocks: 0 IO Block: 131072 regular empty
> file
> Device: 21h/33d Inode: 36596524Links: 1
> Access: (0644/-rw-r--r--) Uid: ( 1049/ jwakely) Gid: ( 1049/ jwakely)
> Access: 2021-03-08 10:52:06.855991946 +
> Modify: 1969-12-31 23:39:59.98500 +
> Change: 2021-03-08 10:52:06.859992051 +
> Birth: -
>
> This is an ext4 filesystem.
>
> Do I need to compile a 64-bit executable?
>
Yes. you need 64bit userland.
> In any case, shouldn't this be fixed in glibc to return EINVAL instead
> of setting a bogus time? That would make the std::filesystem library
> report an error.
>
Since kernel has introduced statx(2), so we can use statx to implemente stat(3).
>
> >Maybe sparc also suffers from this problem, but I have no machine to
> >verify.
>
> Your test case works correctly for me on sparc-linux (both 32-bit and
> 64-bit on ext4 and xfs filesystems):
>
> $ uname -a
> Linux gcc202 5.10.0-4-sparc64-smp #1 SMP Debian 5.10.19-1 (2021-03-02)
> sparc64 GNU/Linux
> $ apt list libc6
> Listing... Done
> libc6/unstable,now 2.31-4 sparc64 [installed]
> $ ./a.out
> $ TZ= stat test
>File: test
>Size: 0 Blocks: 0 IO Block: 8192 regular empty
> file
> Device: fd01h/64769dInode: 657291843 Links: 1
> Access: (0644/-rw-r--r--) Uid: ( 1049/ jwakely) Gid: ( 1049/ jwakely)
> Access: 2021-03-08 10:25:43.593819539 +
> Modify: 1969-12-31 23:39:59.98500 +
> Change: 2021-03-08 10:34:13.711287778 +
> Birth: 2021-03-08 10:24:27.106598699 +
>
--
YunQiang Su
Re: [PATCH] skip testing time before epoch on mips
Chen Li 于2021年3月8日周一 下午2:35写道：
>
>
> When execute libstdc++ testcases on mips, I notice that last_write_time
> alawys failed, and the failed VERIFY is "VERIFY(
> approx_equal(last_write_time(f.path), time) );" where testing time before
> than epoch.
>
> Below is the minimal case:
>
> ```
> // gcc a.c
> int main()
> {
> struct timespec times[2] = {{1, UTIME_OMIT}, {-1201, 98500}};
> utimensat(AT_FDCWD, "test", times, 0);
>
> }
> ```
>
> $ touch test && gcc a.c && ./a.out && stat test
> File: test
> Size: 0 Blocks: 0 IO Block: 4096 regular empty file
> Device: 805h/2053d Inode: 1056841 Links: 1
> Access: (0644/-rw-r--r--) Uid: ( 1000/ deepin) Gid: ( 1000/ deepin)
> Access: 2021-03-08 13:52:55.966354501 +0800
> Modify: 2106-02-07 14:08:15.98500 +0800
> Change: 2021-03-08 13:52:56.907782193 +0800
> Birth: -
>
> Undoubtedly, mtime's type is unsigned somewhere on mips.
>
It only has this problem on mips64.
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=983769
> After debuging kernel, it turns out that mtime is always -1201 in
> ext4_setattr, cp_new_stat, newlstat and etc, so the problem should not
> occur in kernel space.
>
> go back to user space via copy_to_user, I finally found mips used
> "unsigned int st_mtime_sec;" in struct kernel_stat, which is used to
> receive -1201 from kernel.
>
> Maybe sparc also suffers from this problem, but I have no machine to
> verify.
> ---
> .../testsuite/27_io/filesystem/operations/last_write_time.cc| 2 ++
> .../experimental/filesystem/operations/last_write_time.cc | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git
> a/libstdc++-v3/testsuite/27_io/filesystem/operations/last_write_time.cc
> b/libstdc++-v3/testsuite/27_io/filesystem/operations/last_write_time.cc
> index 6ae64c482b6..72272d4e781 100644
> --- a/libstdc++-v3/testsuite/27_io/filesystem/operations/last_write_time.cc
> +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/last_write_time.cc
> @@ -200,11 +200,13 @@ test02()
>VERIFY( approx_equal(last_write_time(f.path), time) );
>
>ec = bad_ec;
> + #ifndef __mips__
>// A time before than the epoch
>time -= chrono::milliseconds(1000 * 60 * 20 + 15);
>last_write_time(f.path, time, ec);
>VERIFY( !ec );
>VERIFY( approx_equal(last_write_time(f.path), time) );
> + #endif
> }
>
> int
> diff --git
> a/libstdc++-v3/testsuite/experimental/filesystem/operations/last_write_time.cc
>
> b/libstdc++-v3/testsuite/experimental/filesystem/operations/last_write_time.cc
> index 536ab1f28f6..6fb490a6bc3 100644
> ---
> a/libstdc++-v3/testsuite/experimental/filesystem/operations/last_write_time.cc
> +++
> b/libstdc++-v3/testsuite/experimental/filesystem/operations/last_write_time.cc
> @@ -165,11 +165,13 @@ test02()
>VERIFY( !ec );
>VERIFY( approx_equal(last_write_time(f.path), time) );
>
> + #ifndef __mips__
>ec = bad_ec;
>time -= chrono::milliseconds(1000 * 60 * 10 + 15);
>last_write_time(f.path, time, ec);
>VERIFY( !ec );
>VERIFY( approx_equal(last_write_time(f.path), time) );
> + #endif
> }
>
> int
> --
> 2.30.0
>
>
>
--
YunQiang Su
Re: [PATCH v4 1/2] MIPS: unaligned load: use SImode for SUBREG if OK (PR98996)
Jeff Law via Gcc-patches 于2021年2月26日周五 上午10:57写道： > > > > On 2/23/21 3:05 AM, YunQiang Su wrote: > > It is found by ada s-pack96.adb ftbfs, due to 96bit load: 96 = 64 + 32. > > While the 32bit pair of l r is mark as SUBREG, so they are > > not in SImode, make it fail to find suitable insn. > > > > gcc/ChangeLog: > > > > PR target/98996 > > * config/mips/mips.c (mips_expand_ext_as_unaligned_load): > > If TARGET_64BIT and dest is SUBREG, we check the width, if it > > equal to SImode, we use SImode operation, just like what we are > > doing for REG one. > Thanks. I fixed some minor whitespace issues and installed both patches > in this series. Thank you so much. > jeff > -- YunQiang Su
Re: [PATCH v3 2/2] ada: add 128bit operation to MIPS N32 and N64
Arnaud Charlet 于2021年2月23日周二 下午5:33写道： > > > > > For MIPS N64 and N32: > > > > add GNATRTL_128BIT_PAIRS to LIBGNAT_TARGET_PAIRS > > > > add GNATRTL_128BIT_OBJS to EXTRA_GNATRTL_NONTASKING_OBJS > > > > > > > > gcc/ada/ChangeLog: > > > > PR ada/98996 > > > > * Makefile.rtl: add 128Bit operation file to MIPS > > > > N64 and N32 (LIBGNAT_TARGET_PAIRS, EXTRA_GNATRTL_NONTASKING_OBJS). > > > > > > As for the other recent submit, the ChangeLog file is now generated > > > automatically. > > > > I did generate the ChangeLog by contrib/mklog.py by: > > contrib/mklog.py 0002_MY_PATCH.patch > > and get an empty ChangeLog template. > > > > gcc/ada/ChangeLog: > > > > * ChangeLog: > > * Makefile.rtl: > > > > So, should I keep it as is? > > The ChangeLog file is generated automatically, so you should not touch it. Thank you. I got it. so the the ChangeLog file is generated for the ChangeLog section of commit msg? > > Arno -- YunQiang Su
Re: [PATCH v3 2/2] ada: add 128bit operation to MIPS N32 and N64
Arnaud Charlet 于2021年2月23日周二 下午5:07写道： > > > For MIPS N64 and N32: > > add GNATRTL_128BIT_PAIRS to LIBGNAT_TARGET_PAIRS > > add GNATRTL_128BIT_OBJS to EXTRA_GNATRTL_NONTASKING_OBJS > > > > gcc/ada/ChangeLog: > > PR ada/98996 > > * Makefile.rtl: add 128Bit operation file to MIPS > > N64 and N32 (LIBGNAT_TARGET_PAIRS, EXTRA_GNATRTL_NONTASKING_OBJS). > > As for the other recent submit, the ChangeLog file is now generated > automatically. I did generate the ChangeLog by contrib/mklog.py by: contrib/mklog.py 0002_MY_PATCH.patch and get an empty ChangeLog template. gcc/ada/ChangeLog: * ChangeLog: * Makefile.rtl: So, should I keep it as is? > > The Makefile.rtl change is OK. > > Arno -- YunQiang Su
