| Message ID | 52976AE0.10200@arm.com |
|---|---|
| State | New |
| Headers | show |
On 28/11/13 16:10, Kyrill Tkachov wrote: > Hi all, > > Some ARMv8-A instructions in the vrint* family as well as the vmaxnm and vminnm > ones do not have a conditional variant and have therefore their "predicable" > attribute set to "no". > However we've discovered that they can still end up conditionalised in some > cases because of the arm_cond_branch pattern that can remove a conditional and > conditionalise the next instruction, unless the "conds" attribute forbids it. To > prevent this happeninf with the vrint and vmaxnm, vminnm instructions, this > patch sets the "conds" attribute to "unconditional". > > This was caught in a testcase where the vrinta instruction ended up being > conditionalised (producing vrintagt) which thankfully the assembler caught and > complained. If this had happened on the smin/smax patterns that don't have a > '%?' in their output template, we would have ended silently miscompiling! > > This should go into trunk and the 4.8 branch. > OK both. R. > Tested arm-none-eabi on a model. > > Ok? > > Thanks, > Kyrill > > 2013-11-28 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > * config/arm/iterators.md (vrint_conds): New int attribute. > * config/arm/vfp.md (<vrint_pattern><SDF:mode>2): Set conds attribute. > (smax<mode>3): Likewise. > (smin<mode>3): Likewise. > > 2013-11-28 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > * gcc.target/arm/vrinta-ce.c: New testcase. >
diff --git a/gcc/config/arm/iterators.md b/gcc/config/arm/iterators.md index db1634b..dc4cf0d 100644 --- a/gcc/config/arm/iterators.md +++ b/gcc/config/arm/iterators.md @@ -525,6 +525,10 @@ (UNSPEC_VRINTA "no") (UNSPEC_VRINTM "no") (UNSPEC_VRINTR "yes") (UNSPEC_VRINTX "yes")]) +(define_int_attr vrint_conds [(UNSPEC_VRINTZ "nocond") (UNSPEC_VRINTP "unconditional") + (UNSPEC_VRINTA "unconditional") (UNSPEC_VRINTM "unconditional") + (UNSPEC_VRINTR "nocond") (UNSPEC_VRINTX "nocond")]) + (define_int_attr nvrint_variant [(UNSPEC_NVRINTZ "z") (UNSPEC_NVRINTP "p") (UNSPEC_NVRINTA "a") (UNSPEC_NVRINTM "m") (UNSPEC_NVRINTX "x") (UNSPEC_NVRINTN "n")]) diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md index 22b6325..6d0515a 100644 --- a/gcc/config/arm/vfp.md +++ b/gcc/config/arm/vfp.md @@ -1277,7 +1277,8 @@ "vrint<vrint_variant>%?.<V_if_elem>\\t%<V_reg>0, %<V_reg>1" [(set_attr "predicable" "<vrint_predicable>") (set_attr "predicable_short_it" "no") - (set_attr "type" "f_rint<vfp_type>")] + (set_attr "type" "f_rint<vfp_type>") + (set_attr "conds" "<vrint_conds>")] ) ;; MIN_EXPR and MAX_EXPR eventually map to 'smin' and 'smax' in RTL. @@ -1293,7 +1294,8 @@ (match_operand:SDF 2 "register_operand" "<F_constraint>")))] "TARGET_HARD_FLOAT && TARGET_FPU_ARMV8 <vfp_double_cond>" "vmaxnm.<V_if_elem>\\t%<V_reg>0, %<V_reg>1, %<V_reg>2" - [(set_attr "type" "f_minmax<vfp_type>")] + [(set_attr "type" "f_minmax<vfp_type>") + (set_attr "conds" "unconditional")] ) (define_insn "smin<mode>3" @@ -1302,7 +1304,8 @@ (match_operand:SDF 2 "register_operand" "<F_constraint>")))] "TARGET_HARD_FLOAT && TARGET_FPU_ARMV8 <vfp_double_cond>" "vminnm.<V_if_elem>\\t%<V_reg>0, %<V_reg>1, %<V_reg>2" - [(set_attr "type" "f_minmax<vfp_type>")] + [(set_attr "type" "f_minmax<vfp_type>") + (set_attr "conds" "unconditional")] ) ;; Unimplemented insns: diff --git a/gcc/testsuite/gcc.target/arm/vrinta-ce.c b/gcc/testsuite/gcc.target/arm/vrinta-ce.c new file mode 100644 index 0000000..71c5b3b --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/vrinta-ce.c @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target arm_v8_vfp_ok } */ +/* { dg-options "-O2 -marm -march=armv8-a" } */ +/* { dg-add-options arm_v8_vfp } */ + +double foo (double a) +{ + if (a > 3.0) + return __builtin_round (a); + + return 0.0; +} + +/* { dg-final { scan-assembler-times "vrinta.f64\td\[0-9\]+" 1 } } */ +
Hi all, Some ARMv8-A instructions in the vrint* family as well as the vmaxnm and vminnm ones do not have a conditional variant and have therefore their "predicable" attribute set to "no". However we've discovered that they can still end up conditionalised in some cases because of the arm_cond_branch pattern that can remove a conditional and conditionalise the next instruction, unless the "conds" attribute forbids it. To prevent this happeninf with the vrint and vmaxnm, vminnm instructions, this patch sets the "conds" attribute to "unconditional". This was caught in a testcase where the vrinta instruction ended up being conditionalised (producing vrintagt) which thankfully the assembler caught and complained. If this had happened on the smin/smax patterns that don't have a '%?' in their output template, we would have ended silently miscompiling! This should go into trunk and the 4.8 branch. Tested arm-none-eabi on a model. Ok? Thanks, Kyrill 2013-11-28 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * config/arm/iterators.md (vrint_conds): New int attribute. * config/arm/vfp.md (<vrint_pattern><SDF:mode>2): Set conds attribute. (smax<mode>3): Likewise. (smin<mode>3): Likewise. 2013-11-28 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * gcc.target/arm/vrinta-ce.c: New testcase.