[PATCH] D116403: [IR] Add DebugEntryValues pass

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This patch is a great use of entry values, however I think there's an awkward semantic gap caused by having it in a separate pass to DAE: any undef dbg.value can be promoted to being an entry value. When DAE runs and sets args to be undef we know for certain that the dbg.values being set to undef referred to arguments, but the inverse isn't true, any old undef dbg.value didn't necessarily refer to an argument.

It's probably OK if there's only one dbg.value, as mentioned in the code comments -- however, I don't believe the implementation in this patch counts to see that there's only one dbg.value, it just converts all undef dbg.values to entry values.

In comparison, if this were done in DAE there wouldn't be that uncertainty, plus I think we'd be able to support scenarios where there are other dbg.values of parameter variables, as we would know which dbg.values used to refer to the argument. (Unless I've missed something). I hate to be the reviewer who says "Why not do it completely differently", but wouldn't that be better?

I believe this code should ignore any functions marked "internal", because DAE can change the function signature of those functions, making the parameter-numbers in DILocalVariables not line up with the arguments. See: llvm/test/Transforms/DeadArgElim/dbginfo.ll .



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:393-396
+  if (Op->getOp() != dwarf::DW_OP_LLVM_entry_value) {
+    // TODO: Handle this kind of entry-value expression.
+    Op = ExprCursor.take();
+  }
----------------
What's the motivation behind this change -- it's not covered by any of the tests changed / added, no?


================
Comment at: llvm/lib/Transforms/Utils/DebugEntryValues.cpp:40
+      // FIXME: arg field in DILocalVariable.
+      if (F.arg_size() != 0 && idx < F.arg_size()) {
+        Parameters.insert({DV, FArgs[idx]});
----------------
Would just the `idx < F.arg_size()` clause be sufficient?


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1757
+        UndefValue *U = UndefValue::get((&(*BI))->getType());
+        (&(*BI))->replaceAllUsesWith(U);
+        DbgValueToErase.push_back((&(*BI)));
----------------
Am I right in thinking this RAUWs a dbg.value instruction? I don't believe there should be any users of the dbg.value, as it doesn't produce a Value.


================
Comment at: llvm/test/Transforms/DebugEntryValues/no-param-change.ll:27-30
+attributes #0 = { noinline nounwind uwtable "frame-pointer"="none" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #1 = { "frame-pointer"="none" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #2 = { nofree nosync nounwind readnone speculatable willreturn }
+attributes #3 = { nounwind }
----------------
Any un-necessary attributes should be dropped to reduce future maintenance


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116403