[NFC][DwarfDebug] Prefer explicit to auto type deduction

[djtodoro]

Summary

We should use explicit type instead of auto type deduction when the type is so obvious. In addition, we remove ambiguity, since auto type deduction sometimes is not that intuitive, so that could lead us to some unwanted behavior.

This patch fixes that in the collectCallSiteParameters() from DwarfDebug module.

[dstenb]

Thanks!

[dblaikie]

We should use explicit type instead of auto type deduction when the type is so obvious.

This seems like it goes against the LLVM Style Guide, which says "Don’t “almost always” use auto, but do use auto with initializers like cast<Foo>(...) or other places where the type is already obvious from the context."

I don't think it's super necessary (I wouldn't advocate for someone changing this code to use auto alone, anymore than I advocate for the change to remove auto here), but equally I'm not sure this change was warranted - please revert it.

If you like, you can keep these things as references, rather than pointers, I think that's generally good - I'm all for more references/fewer pointers. And certainly the old code was misleading in that it unnecessarily used reference lifetime extension, and should've instead use `const auto *` if it was going to have these as pointer typed variables.

[djtodoro]

In D79624#2030876, @dblaikie wrote:

We should use explicit type instead of auto type deduction when the type is so obvious.

This seems like it goes against the LLVM Style Guide, which says "Don’t “almost always” use auto, but do use auto with initializers like cast<Foo>(...) or other places where the type is already obvious from the context."

Oh, I see... My intuition went in wrong direction.

I don't think it's super necessary (I wouldn't advocate for someone changing this code to use auto alone, anymore than I advocate for the change to remove auto here), but equally I'm not sure this change was warranted - please revert it.

I wanted to remove ambiguity, but this may not be the best way.
I reverted this. Thanks for the comment.

If you like, you can keep these things as references, rather than pointers, I think that's generally good - I'm all for more references/fewer pointers. And certainly the old code was misleading in that it unnecessarily used reference lifetime extension, and should've instead use `const auto *` if it was going to have these as pointer typed variables.

[dblaikie]

In D79624#2030991, @djtodoro wrote:

In D79624#2030876, @dblaikie wrote:

We should use explicit type instead of auto type deduction when the type is so obvious.

This seems like it goes against the LLVM Style Guide, which says "Don’t “almost always” use auto, but do use auto with initializers like cast<Foo>(...) or other places where the type is already obvious from the context."

Oh, I see... My intuition went in wrong direction.

The style guide doesn't cover the case you're probably thinking of, but everyone tends to agree - 'auto' is great when a type is really long/unnecessary, like an iterator type, etc - which is sort of the opposite end of the spectrum. The /specific/ type there might be fairly non-obvious, but usually what you can do with it ("oh, it's an iterator, great - I'll do iterator things with it") is obvious.

I don't think it's super necessary (I wouldn't advocate for someone changing this code to use auto alone, anymore than I advocate for the change to remove auto here), but equally I'm not sure this change was warranted - please revert it.

I wanted to remove ambiguity, but this may not be the best way.

Honestly, it might not be a bad change/way to write code - I was/am just a bit skittish about the specific motivation. The general idea of "use auto if it makes the code easier to understand" is a very fuzzy/uncertain thing, really.

I reverted this. Thanks for the comment.

Thanks a bunch! I committed the dereferencing part of your fix though ( aa99da5ace4587440973c97a4cd5f486e7bb3c33 ) which is certainly good to do (shouldn't be binding "auto&" to a pointer type if it's obviously a pointer/something readers will want to treat pointer-like). Thanks for that!

[djtodoro]

In D79624#2032030, @dblaikie wrote:

In D79624#2030991, @djtodoro wrote:

In D79624#2030876, @dblaikie wrote:

We should use explicit type instead of auto type deduction when the type is so obvious.

This seems like it goes against the LLVM Style Guide, which says "Don’t “almost always” use auto, but do use auto with initializers like cast<Foo>(...) or other places where the type is already obvious from the context."

Oh, I see... My intuition went in wrong direction.

The style guide doesn't cover the case you're probably thinking of, but everyone tends to agree - 'auto' is great when a type is really long/unnecessary, like an iterator type, etc - which is sort of the opposite end of the spectrum. The /specific/ type there might be fairly non-obvious, but usually what you can do with it ("oh, it's an iterator, great - I'll do iterator things with it") is obvious.

I don't think it's super necessary (I wouldn't advocate for someone changing this code to use auto alone, anymore than I advocate for the change to remove auto here), but equally I'm not sure this change was warranted - please revert it.

I wanted to remove ambiguity, but this may not be the best way.

Honestly, it might not be a bad change/way to write code - I was/am just a bit skittish about the specific motivation. The general idea of "use auto if it makes the code easier to understand" is a very fuzzy/uncertain thing, really.

I reverted this. Thanks for the comment.

Thanks a bunch! I committed the dereferencing part of your fix though ( aa99da5ace4587440973c97a4cd5f486e7bb3c33 ) which is certainly good to do (shouldn't be binding "auto&" to a pointer type if it's obviously a pointer/something readers will want to treat pointer-like). Thanks for that!

Thanks!

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

static void collectCallSiteParameters(const MachineInstr *CallMI,
   auto CalleesMap = MF->getCallSitesInfo();
   auto CallFwdRegsInfo = CalleesMap.find(CallMI);
 
   // There is no information for the call instruction.
   if (CallFwdRegsInfo == CalleesMap.end())
     return;
 
   auto *MBB = CallMI->getParent();
-  const auto &TRI = MF->getSubtarget().getRegisterInfo();
-  const auto &TII = MF->getSubtarget().getInstrInfo();
-  const auto &TLI = MF->getSubtarget().getTargetLowering();
+  const TargetRegisterInfo &TRI = *MF->getSubtarget().getRegisterInfo();
+  const TargetInstrInfo &TII = *MF->getSubtarget().getInstrInfo();
+  const TargetLowering &TLI = *MF->getSubtarget().getTargetLowering();
 
   // Skip the call instruction.
   auto I = std::next(CallMI->getReverseIterator());
 
   FwdRegWorklist ForwardedRegWorklist;
 
   // If an instruction defines more than one item in the worklist, we may run
   // into situations where a worklist register's value is (potentially)
auto getForwardingRegsDefinedByMI = [&](const MachineInstr &MI,
                                           SmallSetVector<unsigned, 4> &Defs) {
     if (MI.isDebugInstr())
       return;
 
     for (const MachineOperand &MO : MI.operands()) {
       if (MO.isReg() && MO.isDef() &&
           Register::isPhysicalRegister(MO.getReg())) {
         for (auto FwdReg : ForwardedRegWorklist)
-          if (TRI->regsOverlap(FwdReg.first, MO.getReg()))
+          if (TRI.regsOverlap(FwdReg.first, MO.getReg()))
             Defs.insert(FwdReg.first);
       }
     }
   };
 
   // Search for a loading value in forwarding registers.
   for (; I != MBB->rend(); ++I) {
     // Skip bundle headers.
for (; I != MBB->rend(); ++I) {
     // Set of worklist registers that are defined by this instruction.
     SmallSetVector<unsigned, 4> FwdRegDefs;
 
     getForwardingRegsDefinedByMI(*I, FwdRegDefs);
     if (FwdRegDefs.empty())
       continue;
 
     for (auto ParamFwdReg : FwdRegDefs) {
-      if (auto ParamValue = TII->describeLoadedValue(*I, ParamFwdReg)) {
+      if (auto ParamValue = TII.describeLoadedValue(*I, ParamFwdReg)) {
         if (ParamValue->first.isImm()) {
           int64_t Val = ParamValue->first.getImm();
           finishCallSiteParams(Val, ParamValue->second,
                                ForwardedRegWorklist[ParamFwdReg], Params);
         } else if (ParamValue->first.isReg()) {
           Register RegLoc = ParamValue->first.getReg();
-          unsigned SP = TLI->getStackPointerRegisterToSaveRestore();
-          Register FP = TRI->getFrameRegister(*MF);
+          unsigned SP = TLI.getStackPointerRegisterToSaveRestore();
+          Register FP = TRI.getFrameRegister(*MF);
           bool IsSPorFP = (RegLoc == SP) || (RegLoc == FP);
-          if (TRI->isCalleeSavedPhysReg(RegLoc, *MF) || IsSPorFP) {
+          if (TRI.isCalleeSavedPhysReg(RegLoc, *MF) || IsSPorFP) {
             MachineLocation MLoc(RegLoc, /*IsIndirect=*/IsSPorFP);
             finishCallSiteParams(MLoc, ParamValue->second,
                                  ForwardedRegWorklist[ParamFwdReg], Params);
           } else {
             // ParamFwdReg was described by the non-callee saved register
             // RegLoc. Mark that the call site values for the parameters are
             // dependent on that register instead of ParamFwdReg. Since RegLoc
             // may be a register that will be handled in this iteration, we