diff mbox

Keep REG_INC note in subreg2 pass

Message ID CACgzC7AsJZbqabD7df1a0-hmCUX8Efez+XDQR2wSz+wAm3Bc_A@mail.gmail.com
State New
Headers show

Commit Message

Zhenqiang Chen Oct. 24, 2013, 8:20 a.m. UTC 
Hi,

REG_INC note is lost in subreg2 pass when resolve_simple_move, which
might lead to wrong dependence for ira. e.g. In function
validate_equiv_mem of ira.c, it checks REG_INC note:

     for (note = REG_NOTES (insn); note; note = XEXP (note, 1))
        if ((REG_NOTE_KIND (note) == REG_INC
             || REG_NOTE_KIND (note) == REG_DEAD)
            && REG_P (XEXP (note, 0))
            && reg_overlap_mentioned_p (XEXP (note, 0), memref))
          return 0;

Without REG_INC note, validate_equiv_mem will return a wrong result.

Refer https://bugs.launchpad.net/gcc-linaro/+bug/1243022 for more
detail about a real case in kernel.

Bootstrap and no make check regression on X86-64 and ARM.

Is it OK for trunk and 4.8?

Thanks!
-Zhenqiang

ChangeLog:
2013-10-24  Zhenqiang Chen  <zhenqiang.chen@linaro.org>

        * lower-subreg.c (resolve_simple_move): Copy REG_INC note.

testsuite/ChangeLog:
2013-10-24  Zhenqiang Chen  <zhenqiang.chen@linaro.org>

        * gcc.target/arm/lp1243022.c: New test.

Comments

Jeff Law Oct. 29, 2013, 6:47 p.m. UTC | #1 
On 10/24/13 02:20, Zhenqiang Chen wrote:
> Hi,
>
> REG_INC note is lost in subreg2 pass when resolve_simple_move, which
> might lead to wrong dependence for ira. e.g. In function
> validate_equiv_mem of ira.c, it checks REG_INC note:
>
>       for (note = REG_NOTES (insn); note; note = XEXP (note, 1))
>          if ((REG_NOTE_KIND (note) == REG_INC
>               || REG_NOTE_KIND (note) == REG_DEAD)
>              && REG_P (XEXP (note, 0))
>              && reg_overlap_mentioned_p (XEXP (note, 0), memref))
>            return 0;
>
> Without REG_INC note, validate_equiv_mem will return a wrong result.
>
> Referhttps://bugs.launchpad.net/gcc-linaro/+bug/1243022  for more
> detail about a real case in kernel.
>
> Bootstrap and no make check regression on X86-64 and ARM.
>
> Is it OK for trunk and 4.8?
>
> Thanks!
> -Zhenqiang
>
> ChangeLog:
> 2013-10-24  Zhenqiang Chen<zhenqiang.chen@linaro.org>
>
>          * lower-subreg.c (resolve_simple_move): Copy REG_INC note.
>
> testsuite/ChangeLog:
> 2013-10-24  Zhenqiang Chen<zhenqiang.chen@linaro.org>
>
>          * gcc.target/arm/lp1243022.c: New test.
This clearly handles adding a note when the destination is a MEM with a 
side effect.  What about cases where the side effect is associated with 
a load from memory rather than a store to memory?


>
>
> lp1243022.patch
>
>
> diff --git a/gcc/lower-subreg.c b/gcc/lower-subreg.c
> index 57b4b3c..e710fa5 100644
> --- a/gcc/lower-subreg.c
> +++ b/gcc/lower-subreg.c
> @@ -1056,6 +1056,22 @@ resolve_simple_move (rtx set, rtx insn)
>   	mdest = simplify_gen_subreg (orig_mode, dest, GET_MODE (dest), 0);
>         minsn = emit_move_insn (real_dest, mdest);
>
> +#ifdef AUTO_INC_DEC
> +      /* Copy the REG_INC notes.  */
> +      if (MEM_P (real_dest) && !(resolve_reg_p (real_dest)
> +				 || resolve_subreg_p (real_dest)))
> +	{
> +	  rtx note = find_reg_note (insn, REG_INC, NULL_RTX);
> +	  if (note)
> +	    {
> +	      if (!REG_NOTES (minsn))
> +		REG_NOTES (minsn) = note;
> +	      else
> +		add_reg_note (minsn, REG_INC, note);
> +	    }
> +	}
> +#endif
If MINSN does not have any notes, then this results in MINSN and INSN 
sharing the note.  Note carefully that notes are chained (see 
implementation of add_reg_note).  Thus the sharing would result in MINSN 
and INSN actually sharing a chain of notes.  I'm pretty sure that's not 
what you intended.  I think you need to always use add_reg_note.

Jeff
diff mbox

Patch

diff --git a/gcc/lower-subreg.c b/gcc/lower-subreg.c
index 57b4b3c..e710fa5 100644
--- a/gcc/lower-subreg.c
+++ b/gcc/lower-subreg.c
@@ -1056,6 +1056,22 @@  resolve_simple_move (rtx set, rtx insn)
 	mdest = simplify_gen_subreg (orig_mode, dest, GET_MODE (dest), 0);
       minsn = emit_move_insn (real_dest, mdest);
 
+#ifdef AUTO_INC_DEC
+      /* Copy the REG_INC notes.  */
+      if (MEM_P (real_dest) && !(resolve_reg_p (real_dest)
+				 || resolve_subreg_p (real_dest)))
+	{
+	  rtx note = find_reg_note (insn, REG_INC, NULL_RTX);
+	  if (note)
+	    {
+	      if (!REG_NOTES (minsn))
+		REG_NOTES (minsn) = note;
+	      else
+		add_reg_note (minsn, REG_INC, note);
+	    }
+	}
+#endif
+
       smove = single_set (minsn);
       gcc_assert (smove != NULL_RTX);
 
diff --git a/gcc/testsuite/gcc.target/arm/lp1243022.c b/gcc/testsuite/gcc.target/arm/lp1243022.c
new file mode 100644
index 0000000..91a544d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/lp1243022.c
@@ -0,0 +1,201 @@ 
+/* { dg-do compile { target arm_thumb2 } } */
+/* { dg-options "-O2 -fdump-rtl-subreg2" } */
+
+/* { dg-final { scan-rtl-dump "REG_INC" "subreg2" } } */
+/* { dg-final { cleanup-rtl-dump "subreg2" } } */
+struct device;
+typedef unsigned int __u32;
+typedef unsigned long long u64;
+typedef __u32 __le32;
+typedef u64 dma_addr_t;
+typedef unsigned gfp_t;
+int dev_warn (const struct device *dev, const char *fmt, ...);
+struct usb_bus
+{
+    struct device *controller;
+};
+struct usb_hcd
+{
+    struct usb_bus self;
+};
+struct xhci_generic_trb
+{
+    __le32 field[4];
+};
+union xhci_trb
+{
+    struct xhci_generic_trb generic;
+};
+struct xhci_segment
+{
+    union xhci_trb *trbs;
+    dma_addr_t dma;
+};
+struct xhci_ring
+{
+    struct xhci_segment *first_seg;
+};
+struct xhci_hcd
+{
+    struct xhci_ring *cmd_ring;
+    struct xhci_ring *event_ring;
+};
+struct usb_hcd *xhci_to_hcd (struct xhci_hcd *xhci)
+{
+}
+dma_addr_t xhci_trb_virt_to_dma (struct xhci_segment * seg,
+				 union xhci_trb * trb);
+struct xhci_segment *trb_in_td (struct xhci_segment *start_seg,
+				dma_addr_t suspect_dma);
+xhci_test_trb_in_td (struct xhci_hcd *xhci, struct xhci_segment *input_seg,
+		     union xhci_trb *start_trb, union xhci_trb *end_trb,
+		     dma_addr_t input_dma, struct xhci_segment *result_seg,
+		     char *test_name, int test_number)
+{
+    unsigned long long start_dma;
+    unsigned long long end_dma;
+    struct xhci_segment *seg;
+    start_dma = xhci_trb_virt_to_dma (input_seg, start_trb);
+    end_dma = xhci_trb_virt_to_dma (input_seg, end_trb);
+    {
+        dev_warn (xhci_to_hcd (xhci)->self.controller,
+                  "%d\n", test_number);
+        dev_warn (xhci_to_hcd (xhci)->self.controller,
+                  "Expected seg %p, got seg %p\n", result_seg, seg);
+    }
+}
+xhci_check_trb_in_td_math (struct xhci_hcd *xhci, gfp_t mem_flags)
+{
+    struct
+    {
+        dma_addr_t input_dma;
+        struct xhci_segment *result_seg;
+    }
+    simple_test_vector[] =
+        {
+            {
+                0, ((void *) 0)
+            }
+            ,
+            {
+                xhci->event_ring->first_seg->dma - 16, ((void *) 0)}
+            ,
+            {
+                xhci->event_ring->first_seg->dma - 1, ((void *) 0)}
+            ,
+            {
+                xhci->event_ring->first_seg->dma, xhci->event_ring->first_seg}
+            ,
+            {
+                xhci->event_ring->first_seg->dma + (64 - 1) * 16,
+                xhci->event_ring->first_seg
+            }
+            ,
+            {
+                xhci->event_ring->first_seg->dma + (64 - 1) * 16 + 1, ((void *) 0)}
+            ,
+            {
+                xhci->event_ring->first_seg->dma + (64) * 16, ((void *) 0)}
+            ,
+            {
+                (dma_addr_t) (~0), ((void *) 0)
+            }
+        };
+    struct
+    {
+        struct xhci_segment *input_seg;
+        union xhci_trb *start_trb;
+        union xhci_trb *end_trb;
+        dma_addr_t input_dma;
+        struct xhci_segment *result_seg;
+    }
+    complex_test_vector[] =
+        {
+            {
+                .input_seg = xhci->event_ring->first_seg,.start_trb =
+                xhci->event_ring->first_seg->trbs,.end_trb =
+                &xhci->event_ring->first_seg->trbs[64 - 1],.input_dma =
+                xhci->cmd_ring->first_seg->dma,.result_seg = ((void *) 0),
+            }
+            ,
+            {
+                .input_seg = xhci->event_ring->first_seg,.start_trb =
+                xhci->event_ring->first_seg->trbs,.end_trb =
+                &xhci->cmd_ring->first_seg->trbs[64 - 1],.input_dma =
+                xhci->cmd_ring->first_seg->dma,.result_seg = ((void *) 0),
+            }
+            ,
+            {
+                .input_seg = xhci->event_ring->first_seg,.start_trb =
+                xhci->cmd_ring->first_seg->trbs,.end_trb =
+                &xhci->cmd_ring->first_seg->trbs[64 - 1],.input_dma =
+                xhci->cmd_ring->first_seg->dma,.result_seg = ((void *) 0),
+            }
+            ,
+            {
+                .input_seg = xhci->event_ring->first_seg,.start_trb =
+                &xhci->event_ring->first_seg->trbs[0],.end_trb =
+                &xhci->event_ring->first_seg->trbs[3],.input_dma =
+                xhci->event_ring->first_seg->dma + 4 * 16,.result_seg = ((void *) 0),
+            }
+            ,
+            {
+                .input_seg = xhci->event_ring->first_seg,.start_trb =
+                &xhci->event_ring->first_seg->trbs[3],.end_trb =
+                &xhci->event_ring->first_seg->trbs[6],.input_dma =
+                xhci->event_ring->first_seg->dma + 2 * 16,.result_seg = ((void *) 0),
+            }
+            ,
+            {
+                .input_seg = xhci->event_ring->first_seg,.start_trb =
+                &xhci->event_ring->first_seg->trbs[64 - 3],.end_trb =
+                &xhci->event_ring->first_seg->trbs[1],.input_dma =
+                xhci->event_ring->first_seg->dma + 2 * 16,.result_seg = ((void *) 0),
+            }
+            ,
+            {
+                .input_seg = xhci->event_ring->first_seg,.start_trb =
+                &xhci->event_ring->first_seg->trbs[64 - 3],.end_trb =
+                &xhci->event_ring->first_seg->trbs[1],.input_dma =
+                xhci->event_ring->first_seg->dma + (64 - 4) * 16,.result_seg =
+                ((void *) 0),
+            }
+            ,
+            {
+                .input_seg = xhci->event_ring->first_seg,.start_trb =
+                &xhci->event_ring->first_seg->trbs[64 - 3],.end_trb =
+                &xhci->event_ring->first_seg->trbs[1],.input_dma =
+                xhci->cmd_ring->first_seg->dma + 2 * 16,.result_seg = ((void *) 0),
+            }
+        };
+    unsigned int num_tests;
+    int i, ret;
+    num_tests =
+        (sizeof (simple_test_vector) / sizeof ((simple_test_vector)[0]) +
+         (sizeof (struct
+             {
+         }
+             )));
+    for (i = 0; i < num_tests; i++)
+    {
+        ret =
+            xhci_test_trb_in_td (xhci, xhci->event_ring->first_seg,
+                                 xhci->event_ring->first_seg->trbs,
+                                 &xhci->event_ring->first_seg->trbs[64 - 1],
+                                 simple_test_vector[i].input_dma,
+                                 simple_test_vector[i].result_seg, "Simple", i);
+        if (ret < 0)
+            return ret;
+    }
+    for (i = 0; i < num_tests; i++)
+    {
+        ret =
+            xhci_test_trb_in_td (xhci, complex_test_vector[i].input_seg,
+                                 complex_test_vector[i].start_trb,
+                                 complex_test_vector[i].end_trb,
+                                 complex_test_vector[i].input_dma,
+                                 complex_test_vector[i].result_seg, "Complex", i);
+        if (ret < 0)
+            return ret;
+    }
+}