[SDAG] Use useDebugInstrRef instead of shouldUseDebugInstrRef

[mikolaj-pirog]

shouldUseDebugInstrRef can return different value than useDebugInstrRef, since the first depends on opt level which can change. Inconsistent usage can lead to errors later.

I believe that using should... instead of use... here is a result of a minor error during this: https://github.com/llvm/llvm-project/pull/94149/files#diff-8ec547e1244562c5837ed180dd9bed61b3cd960ef90bb6002ea2db41a67ed693

Notice how before the change InstrRef is assigned value from should... before the opt change. Now, it's done after -- opt change happens here:

bool SelectionDAGISelLegacy::runOnMachineFunction(MachineFunction &MF) {
...
  // Decide what flavour of variable location debug-info will be used, before
  // we change the optimisation level.
  MF.setUseDebugInstrRef(MF.shouldUseDebugInstrRef());
....

  return Selector->runOnMachineFunction(MF);
}

Then runOnMachineFunction uses should..., which after opt change may return different value than it did previously.

[llvmbot]

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-selectiondag

Author: Mikołaj Piróg (mikolaj-pirog)

Changes

shouldUseDebugInstrRef can return different value than useDebugInstrRef, since the first depends on opt level which can change. Inconsistent usage can lead to errors later.

I believe that using should... instead of use... here is a result of a minor error during this: https://github.com/llvm/llvm-project/pull/94149/files#diff-8ec547e1244562c5837ed180dd9bed61b3cd960ef90bb6002ea2db41a67ed693

Notice how before the change InstrRef is assigned value from should... before the opt change. Now, it's done after -- opt change happens here:

bool SelectionDAGISelLegacy::runOnMachineFunction(MachineFunction &MF) {
...
  // Decide what flavour of variable location debug-info will be used, before
  // we change the optimisation level.
  MF.setUseDebugInstrRef(MF.shouldUseDebugInstrRef());
....

  return Selector->runOnMachineFunction(MF);
}

Then runOnMachineFunction uses should..., which after opt change may return different value than it did previously.

---

Full diff: https://github.com/llvm/llvm-project/pull/160686.diff

1 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (+1-1)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index e61558c59bf0d..214edfc7c07f3 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -566,7 +566,7 @@ bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) {
   SwiftError->setFunction(mf);
   const Function &Fn = mf.getFunction();
 
-  bool InstrRef = mf.shouldUseDebugInstrRef();
+  bool InstrRef = mf.useDebugInstrRef();
 
   FuncInfo->set(MF->getFunction(), *MF, CurDAG);
 

[e-kud]

I think it is worth mentioning that the main problem happens while using opt-bisect. When the optimization level changes.
My concern here is whether we need shouldUseDebugInstrRef at all? Or maybe we need to check whether we are bisecting in shouldUseDebugInstrRef and return a consistent result for it?

UPD: We need some testing as well...

[mikolaj-pirog]

I think it is worth mentioning that the main problem happens while using opt-bisect. When the optimization level changes. My concern here is whether we need shouldUseDebugInstrRef at all? Or maybe we need to check whether we are bisecting in shouldUseDebugInstrRef and return a consistent result for it?

UPD: We need some testing as well...

My understanding of the shouldUseDebugInstrRef is that it's a helper that's used to set UseInstrRef; it should never be used outside of initialization (and indeed the usage removed in this PR is the only one)

[e-kud]

I think it is worth mentioning that the main problem happens while using opt-bisect. When the optimization level changes. My concern here is whether we need shouldUseDebugInstrRef at all? Or maybe we need to check whether we are bisecting in shouldUseDebugInstrRef and return a consistent result for it?
UPD: We need some testing as well...

My understanding of the shouldUseDebugInstrRef is that it's a helper that's used to set UseInstrRef; it should never be used outside of initialization (and indeed the usage removed in this PR is the only one)

Oh, I see. I just saw three uses, and they looked very similar to me. But indeed this one should use the attribute that we've just set earlier.

[jmorse]

LGTM, I think this matches my expectations for those functions. The primary reason for this facility existing is that SelectionDAG changed the optimisation-level flags according to things like function attributes. This made it important to a) pick which "mode" of debugging-information was to be in at one point in time, and b) to store that independently of any other information.

(Perhaps things have changed since then; but that's the motivation behind the code).

[e-kud]

LGTM. Thanks!