This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch, avr, PR86635] Fix miscompilation with __memx and libgcc float function __gtsf2


Senthil Kumar Selvaraj writes:

> The below patch fixes a miscompilation of function calls with__memx address space
> arguments.
>
> For code like
>
> extern const  __memx float a;
> extern const float b;
>
> int diff () { return a > b; }
>
> when compiled with -Os, a is never loaded and passed in as an argument
> to the __gtsf2 libgcc function.
>
> Turns out that mov<mode> for the variable a expands into
>
> (insn 8 7 9 2 (parallel [
>             (set (reg:SF 22 r22)
>                 (mem/u/c:SF (reg/f:PSI 47) [1 a+0 S4 A8 AS7]))
>             (clobber (reg:SF 22 r22))
>             (clobber (reg:QI 21 r21))
>             (clobber (reg:HI 30 r30))
>         ]) "test.c":4 36 {xloadsf_A}
>      (expr_list:REG_DEAD (reg/f:PSI 47)
>         (expr_list:REG_UNUSED (reg:HI 30 r30)
>             (expr_list:REG_EQUAL (mem/u/c:SF (symbol_ref:PSI ("a") [flags 0xe40]  <var_decl 0x7fabf444c900 a>) [1 a+0 S4 A8 AS7])
>                 (nil)))))
>
> The ud_dce pass sees this insn and deletes it as reg:SF r22 is both set
> and clobbered.
>
> Georg-Johann pointed out a similar issue (PR63633), and that was fixed
> by introducing a pseudo as the target of set. This patch does the same -
> adds an avr_emit2_fix_outputs for gen functions with 2 operands, that
> detects hard reg conflicts with clobbered regs and substitutes pseudos
> in their place.
>
> The patch also adds a testcase to verify a is actually read. Reg testing
> passed. Ok to commit to trunk?

Sent an out-of-date patch. Here's the right one.
>
> Regards
> Senthil
>
gcc/ChangeLog:

2018-07-25  Senthil Kumar Selvaraj  <senthilkumar.selvaraj@microchip.com>

    * config/avr/avr-protos.h (avr_emit2_fix_outputs): New prototype.
	  * config/avr/avr.c (avr_emit2_fix_outputs): New function.
 	  * config/avr/avr.md (mov<mode>): Wrap gen_xload<mode>_A call
    with avr_emit2_fix_outputs.

gcc/testsuite/ChangeLog:

2018-07-25  Senthil Kumar Selvaraj  <senthilkumar.selvaraj@microchip.com>

 	* gcc.target/avr/torture/pr86635.c: New test.



diff --git gcc/config/avr/avr-protos.h gcc/config/avr/avr-protos.h
index 5622e9035a0..f8db418582e 100644
--- gcc/config/avr/avr-protos.h
+++ gcc/config/avr/avr-protos.h
@@ -135,6 +135,7 @@ regmask (machine_mode mode, unsigned regno)
 }
 
 extern void avr_fix_inputs (rtx*, unsigned, unsigned);
+extern bool avr_emit2_fix_outputs (rtx (*)(rtx,rtx), rtx*, unsigned, unsigned);
 extern bool avr_emit3_fix_outputs (rtx (*)(rtx,rtx,rtx), rtx*, unsigned, unsigned);
 
 extern rtx lpm_reg_rtx;
diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
index 81c35e7fbc2..996d5187c52 100644
--- gcc/config/avr/avr.c
+++ gcc/config/avr/avr.c
@@ -13335,6 +13335,34 @@ avr_emit3_fix_outputs (rtx (*gen)(rtx,rtx,rtx), rtx *op,
   return avr_move_fixed_operands (op, hreg, opmask);
 }
 
+/* Same as avr_emit3_fix_outputs, but for 2 operands */
+bool
+avr_emit2_fix_outputs (rtx (*gen)(rtx,rtx), rtx *op,
+                       unsigned opmask, unsigned rmask)
+{
+  const int n = 2;
+  rtx hreg[n];
+
+  /* It is letigimate for GEN to call this function, and in order not to
+     get self-recursive we use the following static kludge.  This is the
+     only way not to duplicate all expanders and to avoid ugly and
+     hard-to-maintain C-code instead of the much more appreciated RTL
+     representation as supplied by define_expand.  */
+  static bool lock = false;
+
+  gcc_assert (opmask < (1u << n));
+
+  if (lock)
+    return false;
+
+  avr_fix_operands (op, hreg, opmask, rmask);
+
+  lock = true;
+  emit_insn (gen (op[0], op[1]));
+  lock = false;
+
+  return avr_move_fixed_operands (op, hreg, opmask);
+}
 
 /* Worker function for movmemhi expander.
    XOP[0]  Destination as MEM:BLK
diff --git gcc/config/avr/avr.md gcc/config/avr/avr.md
index e619e695418..033a428e9f3 100644
--- gcc/config/avr/avr.md
+++ gcc/config/avr/avr.md
@@ -672,7 +672,14 @@
              ; insn-emit does not depend on the mode, it's all about operands.  */
           emit_insn (gen_xload8qi_A (dest, src));
         else
-          emit_insn (gen_xload<mode>_A (dest, src));
+          {
+            operands[0] = dest; operands[1] = src;
+            if (!avr_emit2_fix_outputs (gen_xload<mode>_A, operands, 1 << 0,
+                                      regmask (<MODE>mode, 22)
+                                               | regmask (QImode, 21)
+                                               | regmask (HImode, REG_Z)))
+              FAIL;
+          }
 
         DONE;
       }
diff --git gcc/testsuite/gcc.target/avr/torture/pr86635.c gcc/testsuite/gcc.target/avr/torture/pr86635.c
new file mode 100644
index 00000000000..f91367f7e7a
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/torture/pr86635.c
@@ -0,0 +1,9 @@
+/* { dg-do compile { target { ! avr_tiny } } } */
+/* { dg-options "-std=gnu99" } */
+
+extern const __memx float a;
+extern const float b;
+
+unsigned long diff () { return a > b; }
+
+/* { dg-final { scan-assembler "call __xload_4" } } */


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]