[PATCH] D90046: [LiveDebugValues] Handle spill locations with a fixed and scalable component.

[Jeremy Morse via Phabricator via llvm-commits]

jmorse accepted this revision.
jmorse added a comment.
This revision is now accepted and ready to land.

> I guess that is possible. As long as both frame objects have the same Stack-ID, StackSlotColouring can merge them together. However, in that case, the sizes and offsets of those objects would both be scalable and comparing these sizes would be no different as they would be for non-scalable sizes.

I imagine if they're merged, all the load and store instructions will refer to the same %stack.[0-9]+ frame index. So it's probably fine.

Generally LGTM with the extra CHECK mentioned inline.



================
Comment at: llvm/test/CodeGen/AArch64/live-debugvalues-sve.mir:148-150
+    ADJCALLSTACKUP 0, 0, implicit-def dead $sp, implicit $sp, debug-location !34
+    renamable $z2 = COPY $z0, debug-location !34
+    B %bb.3, debug-location !35
----------------
sdesmalen wrote:
> jmorse wrote:
> > I'd recommend also testing restoring the vector to a register, to ensure LiveDebugValues chops the spill-expression off correctly. VarLocBasedLDV should put in a DBG_VALUE after loading from the stack; you should need to clobber the stack location with some other value to force InstrRefBasedLDV to issue another DBG_VALUE.
> I tried this, but I'm not entirely sure the result is correct because `isRestoreInstruction` always returns false (`TargetInstrInfo::getRestoreSize` always returns `0` if this is not overriden by the target, which it is not for AArch64).
> 
> So when I do a reload from %stack.0 and then clobber that stack location, like this:
> ```$z3 = IMPLICIT_DEF
> renamable $z1 = LD1W_IMM renamable $p0, %stack.0, 0, debug-location !34 :: (load unknown-size from %stack.0, align 16)
> ST1W_IMM renamable $z3, killed renamable $p0, %stack.0, 0 :: (store unknown-size into %stack.0, align 16)
> ```
> 
> LiveDebugValues creates `DBG_VALUE $noreg, $noreg, !27, !DIExpression(), debug-location !30`. It doesn't seem to recognize LD1W_IMM as a reload, and therefore doesn't see that we've just reloaded debug-value `!27` back into `$z1`. The DIExpression is empty though.
> 
> Is that what you meant?
> TargetInstrInfo::getRestoreSize always returns 0 if this is not overriden by the target, which it is not for AArch64

Oooofff, I wasn't aware of that. Making sure restores are correct are probably out of scope for this patch then.

> LiveDebugValues creates DBG_VALUE $noreg,

That's part of the correct behaviour (terminating locations if they're overwritten on the stack), please do add CHECK lines for that, plus a "TODO" indicating this will change when AArch64 can detect loads.


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

https://reviews.llvm.org/D90046