[PATCH] D70431: [DebugInfo] Make describeLoadedValue() reg aware

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1148
+    // zero-extending copy.
+    if (TRI->isSuperRegisterEq(DestReg, Reg) && MI.definesRegister(Reg, TRI))
+      return ParamLoadedValue(*Source, Expr);
----------------
dstenb wrote:
> vsk wrote:
> > vsk wrote:
> > > I think this should either be `TRI->isSuperRegister(...)`, or the previous `Reg == DestReg` condition should be deleted. My preference would be to keep the cases separate, as I find that clearer.
> > Why is it safe to assume the copy would be zero-extending here?
> I looked around at some targets, but I guess there's no contract that says that this is the case. 
> 
> Walking back a bit of what I have done in this patch, maybe the TargetInstrInfo implementation should not attempt to handle such cases, and then we'll leave it up to each target to override the hook for copy instructions that may zero-extend? 
> 
> If so, would it be fine to, at least as a start now that we work towards enabling call site information by default, add an assert that `Reg` is not a super-register of `DestReg`?
> 
> So something like this:
> 
> ```
> if (Reg == DestReg)
>   return ParamLoadedValue(*Source, Expr);
> 
> assert(!TRI->isSuperRegister(DestReg, Reg) && "[...]");
> 
> [...]
> 
> return None;
> ```
> 
> I have not had the time to look into your concern below about sub-registers, but maybe the same should be done for such cases also.
Yeah, I think that's a good idea, ditto for the sub-register case. Adding asserts in the generic code (here) is doubly-nice as it makes it easier for target maintainers to find what's missing.


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1174
+    // zero-extending add immediate.
+    if (TRI->isSuperRegisterEq(DestReg, Reg) && MI.definesRegister(Reg, TRI))
+      return ParamLoadedValue(*Source, Expr);
----------------
dstenb wrote:
> vsk wrote:
> > vsk wrote:
> > > Ditto, not sure why 'isSuperRegister' isn't enough here.
> > Ditto, why is it safe to assume this is a zero-extending add? E.g. I think 'sxta' on ARM works differently.
> Looking around at a few target, it seemed like in such cases the whole extended super-register would be the explicit define, but I guess there's nothing that says that that must be the case.
> 
> I'll look into adding a Register parameter to isAddImmediate(), and let that handle whether or not super-registers can be described (and if so, if the value should be sign-extended).
I see. One alternative might be to document+assert that the generic code cannot handle the described reg being a super-reg of the add destination (& to just defer this work to target-specific describeLoadedValue hooks). Not sure whether that'd be better or much different, just wanted to list the option for completeness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70431