diff mbox series

[MSP430] Fix PR39240 execution failure for msp430-elf

Message ID 55b87e12-fd50-8996-d6a3-bb2399050056@mittosystems.com
State New
Headers show
Series [MSP430] Fix PR39240 execution failure for msp430-elf | expand

Commit Message

Jozef Lawrynowicz May 28, 2018, 11:23 a.m. UTC 
pr39240.c fails at execution at -O1 and above for msp430, due to an erroneous
subreg expression in the zero_extendqisi2 msp430 insn pattern. This causes the
zero extension operation to get optimized out.

The attached patch fixes the insn pattern, and also removes the msp430x ISA
restriction on zero_extendqisi2 and zero_extendhisi2. The assembly instructions
in these patterns can be used in the base MSP430 ISA; they are not MSP430x
specific.

Successfully regtested the GCC testsuite for msp430-elf in the
-mcpu=msp430x/-mlarge configuration.

If the patch is acceptable, I would appreciate if someone would commit it for
me, as I don't have write access.

Comments

Jeff Law May 30, 2018, 11:28 p.m. UTC | #1 
On 05/28/2018 05:23 AM, Jozef Lawrynowicz wrote:
> pr39240.c fails at execution at -O1 and above for msp430, due to an erroneous
> 
> subreg expression in the zero_extendqisi2 msp430 insn pattern. This causes the
> 
> zero extension operation to get optimized out.
> 
> The attached patch fixes the insn pattern, and also removes the msp430x ISA
> restriction on zero_extendqisi2 and zero_extendhisi2. The assembly instructions
> 
> in these patterns can be used in the base MSP430 ISA; they are not MSP430x
> specific.
> 
> Successfully regtested the GCC testsuite for msp430-elf in the
> -mcpu=msp430x/-mlarge configuration.
> 
> If the patch is acceptable, I would appreciate if someone would commit it for
> 
> me, as I don't have write access.
> 
> 
> 0001-MSP430-Fix-PR39240-execution-failure-for-msp430-elf.patch
> 
> 
> From 4f96a05f4849e28064f5c202a55b753b59a106ef Mon Sep 17 00:00:00 2001
> From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
> Date: Sun, 27 May 2018 21:09:49 +0100
> Subject: [PATCH] MSP430: Fix PR39240 execution failure for msp430-elf
> 
> 2018-05-28  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
> 
> 	* gcc/config/msp430/msp430.md: Remove erroneous subreg expression from
> 	zero_extendqisi2 insn pattern. Remove msp430x ISA restriction on
> 	zero_extend{q,h}isi2.
It's not clear why having that subreg is causing incorrect code to be
generated, but the subreg is clearly wrong since it's a qisi pattern not
a hisi pattern.  Based on that I've approved and installed the change.

I'm a big believer that any subreg appearing in an md is fishy and
should be reviewed and justified.  I've found they're generally a bad idea.


Jeff
Jozef Lawrynowicz May 31, 2018, 11:34 a.m. UTC | #2 
On 31/05/18 00:28, Jeff Law wrote:

> It's not clear why having that subreg is causing incorrect code to be
> generated, but the subreg is clearly wrong since it's a qisi pattern not
> a hisi pattern.  Based on that I've approved and installed the change.

There's some further info in PR85941, but I closed that since it didn't appear
combine was at fault.

If that subreg expression is in the insn pattern, then the expand RTL pass is
coerced into adding that additional expression so it is matched as
zero_extendqisi2 in the next pass:

(insn 11 10 12 2 (set (reg:SI 29)
         (zero_extend:SI (subreg:HI (subreg/s/v:QI (reg:HI 23 [ _1 ]) 0) 0)))

By the time we get to the combine pass, the zero extend part looks like:

         (zero_extend:SI (subreg:HI (reg:QI 28) 0)))

The expression to zero_extend then becomes R12:HI, losing the zero extension
from QImode. See below from the combine rtl dump:

insn_cost 4 for     8: r28:QI=R12:QI
       REG_DEAD R12:QI
...
insn_cost 8 for    11: r29:SI=zero_extend(r28:QI#0)
       REG_DEAD r28:QI
...
allowing combination of insns 8 and 11
original costs 4 + 8 = 12
replacement cost 8
deferring deletion of insn with uid = 8.
modifying insn i3    11: r29:SI=zero_extend(R12:HI)
       REG_DEAD R12:QI

I guess combine decided it was valid to just directly use hard reg R12 in HImode
at this point.

> I'm a big believer that any subreg appearing in an md is fishy and
> should be reviewed and justified.  I've found they're generally a bad idea.

Ok, there are quite a few uses of subreg expressions in msp430 insn patterns,
mostly when a PSImode operand is involved. I'll make a note to review these in
the future.

Thanks,
Jozef
diff mbox series

Patch

From 4f96a05f4849e28064f5c202a55b753b59a106ef Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Sun, 27 May 2018 21:09:49 +0100
Subject: [PATCH] MSP430: Fix PR39240 execution failure for msp430-elf

2018-05-28  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	* gcc/config/msp430/msp430.md: Remove erroneous subreg expression from
	zero_extendqisi2 insn pattern. Remove msp430x ISA restriction on
	zero_extend{q,h}isi2.

---
 gcc/config/msp430/msp430.md | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/config/msp430/msp430.md b/gcc/config/msp430/msp430.md
index 869b9ee..614d375 100644
--- a/gcc/config/msp430/msp430.md
+++ b/gcc/config/msp430/msp430.md
@@ -619,15 +619,15 @@ 
 
 (define_insn "zero_extendqisi2"
   [(set (match_operand:SI 0 "nonimmediate_operand" "=r")
-	(zero_extend:SI (subreg:HI (match_operand:QI 1 "nonimmediate_operand" "rm") 0)))]
-  "msp430x"
+	(zero_extend:SI (match_operand:QI 1 "nonimmediate_operand" "rm")))]
+  ""
   "MOV.B\t%1,%L0 { CLR\t%H0"
 )
 
 (define_insn "zero_extendhisi2"
   [(set (match_operand:SI 0 "nonimmediate_operand" "=rm,r")
 	(zero_extend:SI (match_operand:HI 1 "nonimmediate_operand" "0,r")))]
-  "msp430x"
+  ""
   "@
   MOV.W\t#0,%H0
   MOV.W\t%1,%L0 { MOV.W\t#0,%H0"
-- 
2.7.4