[PATCH] D106408: Allow rematerialization of virtual reg uses

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/test/CodeGen/Thumb2/LowOverheadLoops/memcall.ll:8
+; CHECK-NEXT:    .save {r4, r5, r6, r7, r8, lr}
+; CHECK-NEXT:    push.w {r4, r5, r6, r7, r8, lr}
 ; CHECK-NEXT:    cmp r2, #1
----------------
efriedma wrote:
> rampitec wrote:
> > efriedma wrote:
> > > This looks like a regression.  Not sure what's happening here; we're saving both r7 and r8, but they aren't used.  Maybe something related to the hardware loop instructions?
> > Here is what happens, these two instructions are now `isTriviallyReMaterializable()`:
> > ```
> >   %16:rgpr = t2ADDri %8:rgpr, 15, 14, $noreg, $noreg
> >   %17:rgpr = t2LSRri %16:rgpr, 4, 14, $noreg, $noreg
> > ```
> > MachineLICM hoists these out of the loop because of that. RA uses r8 and r8 ends up in the frame setup:
> > ```
> > bb.0.entry:
> >   successors: %bb.1(0x50000000), %bb.2(0x30000000); %bb.1(62.50%), %bb.2(37.50%)
> >   liveins: $r0, $r1, $r2, $r3, $r4, $r5, $r6, $r7, $r8, $lr
> >   $sp = frame-setup t2STMDB_UPD $sp(tied-def 0), 14, $noreg, killed $r4, killed $r5, killed $r6, killed $r7, killed $r8, killed $lr
> >   frame-setup CFI_INSTRUCTION def_cfa_offset 24
> >   frame-setup CFI_INSTRUCTION offset $lr, -4
> >   frame-setup CFI_INSTRUCTION offset $r8, -8
> >   frame-setup CFI_INSTRUCTION offset $r7, -12
> >   frame-setup CFI_INSTRUCTION offset $r6, -16
> >   frame-setup CFI_INSTRUCTION offset $r5, -20
> >   frame-setup CFI_INSTRUCTION offset $r4, -24
> >   t2CMPri renamable $r2, 1, 14, $noreg, implicit-def $cpsr
> >   t2Bcc %bb.2, 11, killed $cpsr
> >   t2B %bb.1, 14, $noreg
> > 
> > bb.1.for.body.preheader:
> > ; predecessors: %bb.0
> >   successors: %bb.3(0x80000000); %bb.3(100.00%)
> >   liveins: $r0, $r1, $r2, $r3
> >   renamable $r12 = nsw t2LSLri renamable $r3, 2, 14, $noreg, $noreg
> >   renamable $r4 = t2MOVi 0, 14, $noreg, $noreg
> >   renamable $r7 = t2ADDri renamable $r3, 15, 14, $noreg, $noreg
> >   renamable $r8 = t2LSRri killed renamable $r7, 4, 14, $noreg, $noreg
> >   t2B %bb.3, 14, $noreg
> > ```
> > But it is eliminated by the ARM Low Overhead Loops pass:
> > ```
> > # *** IR Dump After ARM Low Overhead Loops pass (arm-low-overhead-loops) ***:
> > 
> > bb.1.for.body.preheader:
> > ; predecessors: %bb.0
> >   successors: %bb.2(0x80000000); %bb.2(100.00%)
> >   liveins: $r0, $r1, $r2, $r3
> >   renamable $r12 = nsw t2LSLri renamable $r3, 2, 14, $noreg, $noreg
> >   renamable $r4, dead $cpsr = tMOVi8 0, 14, $noreg
> >   tB %bb.2, 14, $noreg
> > ```
> > Frame setup however already done and not updated.
> > 
> > Maybe pass an extra argument into `isTriviallyReMaterializable()`? That could return false if any virtual registers are used for the purpose of MachineLICM and CalcSpillWeights and only return true for the regalloc/coalescer itself.
> That's unfortunate.
> 
> In general, the hoisting is probably fine.  The problem here is that if the instructions are used as input to the low-overhead loop pseudo-instructions, we don't want to hoist them: they're likely to be eliminated by the LowOverheadLoops pass, so it isn't profitable.  (The low-overhead loop instructions get formed very late because they have odd restrictions on what branches are allowed.)
I am not really sure it is fine to hoist in this case because a rematerialization of an instruction with vreg uses is not guaranteed. All used registers must be available at the point of rematerialization and if not it simply will not happen. MachineLICM considers rematerialization as granted, but in this case it is rather opportunistic.

I am looking at the change not to hoist such instructions right now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106408/new/

https://reviews.llvm.org/D106408