[PATCH] D140900: [DebugInfo] Print empty MDTuples wrapped in MetadataAsValue inline

Mon Mar 13 05:01:36 PDT 2023

jmorse added a comment.

I agree with Stephen, it's good but flipping the FromValue flag doesn't seem right.

================
Comment at: llvm/lib/IR/AsmWriter.cpp:4861

-  WriteAsOperandInternal(OS, &MD, *WriterCtx, /* FromValue */ true);
+  WriteAsOperandInternal(OS, &MD, *WriterCtx, /* FromValue */ false);

----------------
StephenTozer wrote:
> Setting this to `false`, while it makes total sense to me, caused an error in the LLDB test suite last time it was done[0][1]. Off the top of my head I'm not sure under what circumstances exactly we're calling `printMetadataImpl` directly for a `LocalAsMetadata` or `DIArgList`, but (much as a pain as it is) it probably needs to be verified that this error won't come up again - happy to investigate further myself if needed.
> 
> [0] https://reviews.llvm.org/D104827#2856347
> [1] https://reviews.llvm.org/D104827#2898216
Yeah, changing this feels at risk of being overkill -- I can understand why we want the condition that `FromValue` is true when printing inline, but why does this call-site need to change for that? Possibly printMetadataImpl needs to take a FromValue argument itself and push the decision making higher up the call stack?

================
Comment at: llvm/test/DebugInfo/Generic/empty-metadata-roundtrip.ll:6
+;; though it's not used elsewhere. This is to avoid adding more context to the
+;; print functions, but it's is not a requirement so it is okay to update this
+;; test if that changes in the future.
----------------
Worth using the term "cosmetic" to fully communicate that there's no functional impact of this behaviour?

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

https://reviews.llvm.org/D140900