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

[David Stenberg via Phabricator via llvm-commits]

dstenb marked an inline comment as done.
dstenb 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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70431