[PATCH] D114355: [DebugInfo] ValueMapper impl for DIArgList should respect RF_IgnoreMissingLocals

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

> This patch fixes an issue in which SSA value reference within a DIArgList would be unnecessarily dropped by llvm-link, even when invoking on a single file (which should be a no-op).

Please add a couple of IR-based tests for `llvm-link` in addition to the unit test (I suggest at least one that's single file and one that's multi-file).



================
Comment at: llvm/lib/Transforms/Utils/ValueMapper.cpp:411
+        } else if ((Flags & RF_IgnoreMissingLocals) && isa<LocalAsMetadata>(VAM)) {
+            MappedArgs.push_back(VAM);
         } else {
----------------
It'd be great to have a comment here explaining why `undef` isn't correct, and what's happening instead. It's not obvious to me how identity could be valid, given that the local doesn't exist on the other side... to me it looks like this could cause a `DIArgList` from one llvm::Function to point at a local llvm::Value owned by a different function (or module). Is that possibility excluded somehow? How? (the test suggests this results in `metadata !{}`; should there be a comment explaining the mechanism for that here?)

>From the commit message, the goal is to change LocalAsMetadata that's referenced from a DIArgList, but it looks like it changes behaviour for all LocalAsMetadata. Can you explain why that's okay/safe?


================
Comment at: llvm/unittests/Transforms/Utils/ValueMapperTest.cpp:295
   auto *LAM = LocalAsMetadata::get(&A);
-  auto *MAV = MetadataAsValue::get(C, LAM);
+  auto *MAV0 = MetadataAsValue::get(C, LAM);
+  std::vector<ValueAsMetadata*> Elts;
----------------
This change makes it hard for me to see what has changed and what hasn't in the test. Please do one of the following:
- commit NFC churn separately as a prep commit and rebase
- avoid churn by choosing a different naming scheme that leaves old code untouched
- avoid churn by moving new test code to a separate test (maybe `mapValueDIArgList`)


================
Comment at: llvm/unittests/Transforms/Utils/ValueMapperTest.cpp:309-310
   // return here, but we also shouldn't reference the unmapped local.  Use
-  // "metadata !{}".
+  // "metadata !{}" for direct LocalAsMetadata, and undef for uses in a
+  // DIArgList.
   auto *N0 = MDTuple::get(C, None);
----------------
This comment makes it look like the behaviour changed for DIArgList, but the code change appears to affect only LocalAsMetadata. Can you explain what I'm missing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114355