[PATCH PR94442] [AArch64] Redundant ldp/stp instructions emitted at -O3

Richard Sandiford richard.sandiford@arm.com
Fri Aug 21 09:02:27 GMT 2020


xiezhiheng <xiezhiheng@huawei.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
>> Sent: Thursday, August 20, 2020 4:55 PM
>> To: xiezhiheng <xiezhiheng@huawei.com>
>> Cc: Richard Biener <richard.guenther@gmail.com>; gcc-patches@gcc.gnu.org
>> Subject: Re: [PATCH PR94442] [AArch64] Redundant ldp/stp instructions
>> emitted at -O3
>> 
>> xiezhiheng <xiezhiheng@huawei.com> writes:
>> >> -----Original Message-----
>> >> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
>> >> Sent: Wednesday, August 19, 2020 6:06 PM
>> >> To: xiezhiheng <xiezhiheng@huawei.com>
>> >> Cc: Richard Biener <richard.guenther@gmail.com>;
>> gcc-patches@gcc.gnu.org
>> >> Subject: Re: [PATCH PR94442] [AArch64] Redundant ldp/stp instructions
>> >> emitted at -O3
>> >>
>> >> xiezhiheng <xiezhiheng@huawei.com> writes:
>> >> > I add FLAGS for part of intrinsics in aarch64-simd-builtins.def first for a
>> try,
>> >> > including all the add/sub arithmetic intrinsics.
>> >> >
>> >> > Something like faddp intrinsic which only handles floating-point
>> operations,
>> >> > both FP and NONE flags are suitable for it because FLAG_FP will be
>> added
>> >> > later if the intrinsic handles floating-point operations.  And I prefer FP
>> >> since
>> >> > it would be more clear.
>> >>
>> >> Sounds good to me.
>> >>
>> >> > But for qadd intrinsics, they would modify FPSR register which is a
>> scenario
>> >> > I missed before.  And I consider to add an additional flag
>> >> FLAG_WRITE_FPSR
>> >> > to represent it.
>> >>
>> >> I don't think we make any attempt to guarantee that the Q flag is
>> >> meaningful after saturating intrinsics.  To do that, we'd need to model
>> >> the modification of the flag in the .md patterns too.
>> >>
>> >> So my preference would be to leave this out and just use NONE for the
>> >> saturating forms too.
>> >
>> > The problem is that the test case in the attachment has different results
>> under -O0 and -O2.
>> 
>> Right.  But my point was that I don't think that use case is supported.
>> If you want to use saturating instructions and read the Q flag afterwards,
>> the saturating instructions need to be inline asm too.
>> 
>> > In gimple phase statement:
>> >   _9 = __builtin_aarch64_uqaddv2si_uuu (op0_4, op1_6);
>> > would be treated as dead code if we set NONE flag for saturating intrinsics.
>> > Adding FLAG_WRITE_FPSR would help fix this problem.
>> >
>> > Even when we set FLAG_WRITE_FPSR, the uqadd insn:
>> >   (insn 11 10 12 2 (set (reg:V2SI 97)
>> >         (us_plus:V2SI (reg:V2SI 98)
>> >             (reg:V2SI 99))) {aarch64_uqaddv2si}
>> >      (nil))
>> > could also be eliminated in RTL phase because this insn will be treated as
>> dead insn.
>> > So I think we might also need to modify saturating instruction patterns
>> adding the side effect of set the FPSR register.
>> 
>> The problem is that FPSR is global state and we don't in general
>> know who might read it.  So if we modelled the modification of the FPSR,
>> we'd never be able to fold away saturating arithmetic that does actually
>> saturate at compile time, because we'd never know whether the program
>> wanted the effect on the Q flag result to be visible (perhaps to another
>> function that the compiler can't see).  We'd also be unable to remove
>> results that really are dead.
>> 
>> So I think this is one of those situations in which we can't keep all
>> constituents happy.  Catering for people who want to read the Q flag
>> would make things worse for those who want saturating arithmetic to be
>> optimised as aggressively as possible.  And the same holds in reverse.
>
> I agree.  The test case is extracted from gcc.target/aarch64/advsimd-intrinsics/vqadd.c
> If we set NONE flag for saturating intrinsics, it would fail in regression because some qadd
> intrinsics would be treated as dead code and be eliminated.
>   Running target unix
>   Running ./gcc.target/aarch64/advsimd-intrinsics/advsimd-intrinsics.exp ...
>   PASS: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O0  (test for excess errors)
>   PASS: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O0  execution test
>   PASS: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O1  (test for excess errors)
>   FAIL: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O1  execution test
>   PASS: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O2  (test for excess errors)
>   FAIL: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O2  execution test
>   PASS: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O3 -g  (test for excess errors)
>   FAIL: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O3 -g  execution test
>   PASS: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -Os  (test for excess errors)
>   FAIL: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -Os  execution test
>   PASS: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -Og -g  (test for excess errors)
>   FAIL: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -Og -g  execution test
>   PASS: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  (test for excess errors)
>   FAIL: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
>   PASS: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  (test for excess errors)
>   FAIL: gcc.target/aarch64/advsimd-intrinsics/vqadd.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  execution test

Ah, OK.

> So maybe this test case should only be tested at -O0 level?

Looks like the saturating intrinsics might need a bit more thought.
Would you mind submitting the patch with just the other parts?
Those were uncontroversial and it would be a shame to hold them
up over this.

Thanks,
Richard


More information about the Gcc-patches mailing list