| Message ID | 546DAAB3.1040307@arm.com |
|---|---|
| State | New |
| Headers | show |
> On 19/11/14 09:29, Yangfei (Felix) wrote: > >>> Sorry for missing the point. It seems to me that 't2' here will > >>> conflict with > >> condition of the pattern *movhi_insn_arch4: > >>> "TARGET_ARM > >>> && arm_arch4 > >>> && (register_operand (operands[0], HImode) > >>> || register_operand (operands[1], HImode))" > >>> > >>> #define TARGET_ARM (! TARGET_THUMB) > >>> /* 32-bit Thumb-2 code. */ > >>> #define TARGET_THUMB2 (TARGET_THUMB && > >> arm_arch_thumb2) > >>> > >> > >> Bah, Indeed ! - I misremembered the t2 there, my mistake. > >> > >> Yes you are right there, but what I'd like you to do is to use that > >> mechanism rather than putting all this logic in the predicate. > >> > >> So, I'd prefer you to add a v6t2 to the values for the "arch" > >> attribute, don't forget to update the comments above. > >> > >> and in arch_enabled you need to enforce this with > >> > >> (and (eq_attr "arch" "v6t2") > >> (match_test "TARGET_32BIT && arm_arch6 && > arm_arch_thumb2")) > >> (const_string "yes") > >> > >> And in the pattern use v6t2 ... > >> > >> arm_arch_thumb2 implies that this is at the architecture level of v6t2. > >> Therefore TARGET_ARM && arm_arch_thumb2 implies ARM state. > > > > > > Hi Ramana, > > Thank you for your suggestions. I rebased the patch on the latest trunk > and updated it accordingly. > > As this patch will not work for architectures older than armv6t2, I also > prefer Thomas's patch to fix for them. > > I am currently performing test for this patch. Assuming no issues pops > up, OK for the trunk? > > And is it necessary to backport this patch to the 4.8 & 4.9 branches? > > > > I've applied the following as obvious after Kugan mentioned on IRC this morning > noticing a movwne r0, #-32768. Obviously this won't be accepted as is by the > assembler and we should be using the %L character. Applied to trunk as obvious. > > Felix, How did you test this patch ? > > regards > Ramana I regtested the patch for arm-eabi-gcc/g++ & big-endian with qemu. The test result is OK. That's strange ... This issue can be reproduced by the following testcase. Thanks for fixing it. #include <stdio.h> unsigned short v = 0x5678; int i; int j = 0; int *ptr = &j; int func() { for (i = 0; i < 1; ++i) { *ptr = -1; v = 0xF234; } return v; } > > 2014-11-20 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> > > PR target/59593 > * config/arm/arm.md (*movhi_insn): Use right formatting > for immediate.
> > On 19/11/14 09:29, Yangfei (Felix) wrote: > > >>> Sorry for missing the point. It seems to me that 't2' here will > > >>> conflict with > > >> condition of the pattern *movhi_insn_arch4: > > >>> "TARGET_ARM > > >>> && arm_arch4 > > >>> && (register_operand (operands[0], HImode) > > >>> || register_operand (operands[1], HImode))" > > >>> > > >>> #define TARGET_ARM (! TARGET_THUMB) > > >>> /* 32-bit Thumb-2 code. */ > > >>> #define TARGET_THUMB2 (TARGET_THUMB && > > >> arm_arch_thumb2) > > >>> > > >> > > >> Bah, Indeed ! - I misremembered the t2 there, my mistake. > > >> > > >> Yes you are right there, but what I'd like you to do is to use that > > >> mechanism rather than putting all this logic in the predicate. > > >> > > >> So, I'd prefer you to add a v6t2 to the values for the "arch" > > >> attribute, don't forget to update the comments above. > > >> > > >> and in arch_enabled you need to enforce this with > > >> > > >> (and (eq_attr "arch" "v6t2") > > >> (match_test "TARGET_32BIT && arm_arch6 && > > arm_arch_thumb2")) > > >> (const_string "yes") > > >> > > >> And in the pattern use v6t2 ... > > >> > > >> arm_arch_thumb2 implies that this is at the architecture level of v6t2. > > >> Therefore TARGET_ARM && arm_arch_thumb2 implies ARM state. > > > > > > > > > Hi Ramana, > > > Thank you for your suggestions. I rebased the patch on the > > > latest trunk > > and updated it accordingly. > > > As this patch will not work for architectures older than > > > armv6t2, I also > > prefer Thomas's patch to fix for them. > > > I am currently performing test for this patch. Assuming no > > > issues pops > > up, OK for the trunk? > > > And is it necessary to backport this patch to the 4.8 & 4.9 branches? > > > > > > > I've applied the following as obvious after Kugan mentioned on IRC > > this morning noticing a movwne r0, #-32768. Obviously this won't be > > accepted as is by the assembler and we should be using the %L character. > Applied to trunk as obvious. > > > > Felix, How did you test this patch ? > > > > regards > > Ramana > > > I regtested the patch for arm-eabi-gcc/g++ & big-endian with qemu. The test > result is OK. That's strange ... > > This issue can be reproduced by the following testcase. Thanks for fixing it. > > #include <stdio.h> > unsigned short v = 0x5678; > int i; > int j = 0; > int *ptr = &j; > int func() > { > for (i = 0; i < 1; ++i) > { > *ptr = -1; > v = 0xF234; > } > return v; > } > And the architecture level is set to armv7-a by default when testing.
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md index 3a6e0b0..a52716d 100644 --- a/gcc/config/arm/arm.md +++ b/gcc/config/arm/arm.md @@ -6302,7 +6302,7 @@ "@ mov%?\\t%0, %1\\t%@ movhi mvn%?\\t%0, #%B1\\t%@ movhi - movw%?\\t%0, %1\\t%@ movhi + movw%?\\t%0, %L1\\t%@ movhi str%(h%)\\t%1, %0\\t%@ movhi ldr%(h%)\\t%0, %1\\t%@ movhi" [(set_attr "predicable" "yes")