RE: RE: [PATCH PR95696] regrename creates overlapping register allocations for vliw

[Zhongyunde <zhongyunde@huawei.com>]

[-- Attachment #1: Type: text/plain, Size: 2871 bytes --]


> -----Original Message-----
> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
> Sent: Wednesday, July 22, 2020 12:12 AM
> To: Zhongyunde <zhongyunde@huawei.com>
> Cc: gcc-patches@gcc.gnu.org; Yangfei (A) <lancelo.yang@huawei.com>
> Subject: Re: 答复: [PATCH PR95696] regrename creates overlapping
> register allocations for vliw
> 
> Zhongyunde <zhongyunde@huawei.com> writes:
> > Thanks for your review.
> >
> > First of all, this is an optimization.
> 
> OK, good.
> 
> >    gcc do sms before reload, and here each insn use pseudo-register.
> After reload, they are allocated hard-register, then the regrename pass try
> to adjust the register number with def/use chain created by
> build_def_use.
> >  As now gcc doesn't consider the VLIW bundles, so regrename pass may
> updated a reg which may not really unused, which will bring in invalid
> VLIW bundles.
> >    Before the final schedule, we usually recheck the validation of VLIW
> bundles, and reschedule the conflicted insns into two VLIW to make them
> validation to avoid above issue, so this is not a correctness issue.
> >  Certainly, reschedule the conflicted insns into two VLIW will destroy
> the kernel loop's sms schedule result, and usually it will be harmful to the
> performance.
> 
> Yeah.  The reason I was worried about the TI markers being stale is that, in
> general, register allocation can introduce new spills and reloads, can add
> and remove instructions, and can convert instructions into different forms
> (e.g. as a result of register elimination).
> There are then post-reload optimisers that can change the code further.
> All these things could invalidate the VLIW bundling done by the first
> scheduler.
> 
> It sounds like that's not happening in your motivating testcase, and the
> VLIW bundling is still correct (for this loop) by the time that regrename
> runs.  Is that right?

Yes, it is right.

> It's interesting that this is for a testcase using SMS.  One of the traditional
> problems with the GCC implementation of SMS has been ensuring that
> later passes don't mess up the scheduled loop.  So in your testcase, does
> register allocation succeed for the SMS loop without invalidating the
> bundling decisions?

Yes.

> If so, then it's probably better to avoid running regrename on it at all.
> It mostly exists to help the second scheduling pass, but the second
> scheduling pass shouldn't be messing with an SMS loop anyway.  Also,
> although the patch deals with one case in which regrename could disrupt
> the bundling, there are others too.
> 
> So maybe one option would be to make regrename ignore blocks that
> have BB_DISABLE_SCHEDULE set.  (Sorry if that's been discussed and
> discounted
> already.)

ok, according your advice, I make a new patch attached.

> Thanks,
> Richard

[-- Attachment #2: PR95696_1.patch --]
[-- Type: application/octet-stream, Size: 2274 bytes --]

diff --git a/gcc/regrename.c b/gcc/regrename.c
index c38173a77..0ccc5e18c 100644
--- a/gcc/regrename.c
+++ b/gcc/regrename.c
@@ -284,6 +284,48 @@ create_new_chain (unsigned this_regno, unsigned this_nregs, rtx *loc,
   return head;
 }
 
+/* For the def-use chain HEAD of SMS block, find the registers with REG_UNUSED
+   conflict its VLIW constraint and set the corresponding bits in *PSET.  */
+
+static void
+merge_vliw_overlapping_regs (HARD_REG_SET *pset, struct du_head *head)
+{
+  rtx_insn *insn;
+  rtx_insn *cur_insn = head->first->insn;
+  basic_block bb = BLOCK_FOR_INSN (cur_insn);
+
+  /* Just run regrename for no-SMS related basic block.  */
+  if ((bb->flags & BB_DISABLE_SCHEDULE) == 0 || (bb->flags & BB_MODIFIED) != 0)
+    return;
+
+  /* SMS related basic block generate VLIW before reload, and avoid disrupting
+     the bundling if the new rename reg is valid for bundling.  */
+  for (insn = prev_active_insn (cur_insn); insn && BLOCK_FOR_INSN (insn) == bb;
+       insn = prev_active_insn (insn))
+    {
+      rtx note;
+
+      for (note = REG_NOTES (insn); note; note = XEXP (note, 1))
+        {
+          if (REG_NOTE_KIND (note) == REG_UNUSED)
+          {
+            int regno = REGNO (XEXP (note, 0));
+            SET_HARD_REG_BIT (*pset, regno);
+            if (dump_file)
+              {
+                fprintf (dump_file, "Register %s may confilct with REG_UNUSED \
+                                     in insn %d\n",
+                         reg_names[regno], INSN_UID (insn));
+              }
+          }
+        }
+
+      /* Walk until the first insn of current vliw.  */
+      if (GET_MODE (insn) == TImode)
+        break;
+    }
+}
+
 /* For a def-use chain HEAD, find which registers overlap its lifetime and
    set the corresponding bits in *PSET.  */
 
@@ -372,6 +414,9 @@ find_rename_reg (du_head_p this_head, enum reg_class super_class,
   /* Mark registers that overlap this chain's lifetime as unavailable.  */
   merge_overlapping_regs (unavailable, this_head);
 
+  /* Mark registers that conflict vliw constraint as unavailable.  */
+  merge_vliw_overlapping_regs (unavailable, this_head);
+
   /* Compute preferred rename class of super union of all the classes
      in the chain.  */
   preferred_class