diff mbox series

RISC-V: Standardize formatting of SFB ALU conditional move

Message ID alpine.DEB.2.20.2207261916490.10833@tpp.orcam.me.uk
State New
Headers show
Series RISC-V: Standardize formatting of SFB ALU conditional move | expand

Commit Message

Maciej W. Rozycki July 26, 2022, 6:38 p.m. UTC 
Standardize the formatting of SFB ALU conditional move operations from:

	beq a2,zero,1f; mv a0,zero; 1: # movcc

to:

	beq	a2,zero,1f	# movcc
	mv	a0,zero
1:

for consistency with other assembly code produced.  No functional change.

	gcc/
	* gcc/config/riscv/riscv.md (*mov<GPR:mode><X:mode>cc): Fix 
	output pattern formatting.
---
Hi,

 No regressions in `-mtune=sifive-7-series' testing except for:

FAIL: gcc.target/riscv/pr105314.c   -O0   scan-assembler-not \tbeq\t
FAIL: gcc.target/riscv/pr105314.c   -O1   scan-assembler-not \tbeq\t
FAIL: gcc.target/riscv/pr105314.c   -O2   scan-assembler-not \tbeq\t
FAIL: gcc.target/riscv/pr105314.c   -O3 -g   scan-assembler-not \tbeq\t
FAIL: gcc.target/riscv/pr105314.c   -Os   scan-assembler-not \tbeq\t
FAIL: gcc.target/riscv/pr105314.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none   scan-assembler-not \tbeq\t
FAIL: gcc.target/riscv/pr105314.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects   scan-assembler-not \tbeq\t

as expected due to the brokenness of the test case, as discussed here: 
<https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598542.html> (still 
unresolved).

 OK to apply?

  Maciej
---
 gcc/config/riscv/riscv.md |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

gcc-riscv-movcc-format.diff

Comments

Maciej W. Rozycki Aug. 18, 2022, 2:40 p.m. UTC | #1 
On Tue, 26 Jul 2022, Maciej W. Rozycki wrote:

> Standardize the formatting of SFB ALU conditional move operations from:
> 
> 	beq a2,zero,1f; mv a0,zero; 1: # movcc
> 
> to:
> 
> 	beq	a2,zero,1f	# movcc
> 	mv	a0,zero
> 1:
> 
> for consistency with other assembly code produced.  No functional change.

 Ping for: 
<https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598854.html>

  Maciej
Kito Cheng Aug. 18, 2022, 2:47 p.m. UTC | #2 
OK, thanks for tweaking this!

On Thu, Aug 18, 2022 at 10:40 PM Maciej W. Rozycki <macro@embecosm.com> wrote:
>
> On Tue, 26 Jul 2022, Maciej W. Rozycki wrote:
>
> > Standardize the formatting of SFB ALU conditional move operations from:
> >
> >       beq a2,zero,1f; mv a0,zero; 1: # movcc
> >
> > to:
> >
> >       beq     a2,zero,1f      # movcc
> >       mv      a0,zero
> > 1:
> >
> > for consistency with other assembly code produced.  No functional change.
>
>  Ping for:
> <https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598854.html>
>
>   Maciej
Maciej W. Rozycki Aug. 18, 2022, 3:54 p.m. UTC | #3 
On Thu, 18 Aug 2022, Kito Cheng wrote:

> OK, thanks for tweaking this!

 Committed now, thanks for your review!

 Would you mind sharing your opinion on my previous observation here:
<https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598542.html>?

 I have since realised we have a `-mbranch-cost=' option letting the user 
set the threshold for choosing branches over alternative code sequences, 
so my concern is valid even for our tree unchanged and without the change 
just committed here applied.  Consequently the test case may fail.

 E.g. with:

RUNTESTFLAGS="--target_board remote-unmatched/-mbranch-cost=1 riscv.exp=pr105314.c"

I get:

PASS: gcc.target/riscv/pr105314.c   -O0  (test for excess errors)
FAIL: gcc.target/riscv/pr105314.c   -O0   scan-assembler-not \tbeq\t
PASS: gcc.target/riscv/pr105314.c   -O1  (test for excess errors)
FAIL: gcc.target/riscv/pr105314.c   -O1   scan-assembler-not \tbeq\t
[...]

		=== gcc Summary ===

# of expected passes		9
# of unexpected failures	9

because GCC legitimately chooses to emit branches as less costly in this 
case.

 I think we need to pacify the test case somehow if it does not match the 
criteria for PR105314, either by excluding the case from testing in that 
situation or by forcing it via command-line options to make it match the 
criteria (or indeed by verifying a branch is produced regardless).  Sadly 
Jakub chose not to chime in and it's not clear to me which approach would 
be the most appropriate here.

  Maciej
diff mbox series

Patch

Index: gcc/gcc/config/riscv/riscv.md
===================================================================
--- gcc.orig/gcc/config/riscv/riscv.md
+++ gcc/gcc/config/riscv/riscv.md
@@ -2187,8 +2187,8 @@ 
 	 (match_operand:GPR 4 "sfb_alu_operand" "rJ,IL")))]
   "TARGET_SFB_ALU"
   "@
-   b%C5 %1,%z2,1f; mv %0,%z4; 1: # movcc
-   b%C5 %1,%z2,1f; li %0,%4; 1: # movcc"
+   b%C5\t%1,%z2,1f\t# movcc\;mv\t%0,%z4\n1:
+   b%C5\t%1,%z2,1f\t# movcc\;li\t%0,%4\n1:"
   [(set_attr "length" "8")
    (set_attr "type" "sfb_alu")
    (set_attr "mode" "<GPR:MODE>")])