[PATCH] D128177: [DebugInfo][InstrRef][NFC] Represent DbgValues with multiple ops in LDV

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:334
+    } ID;
+    uint32_t RawID;
+  };
----------------
Orlando wrote:
> It looks like you use this union for type punning. IIRC this is UB in C++ but I am unsure. Is this a case that is permitted for some reason, do you know?
I don't know actually - but this file was already using this type of behaviour beforehand in `ValueIDNum`. All the same, it shouldn't be difficult to do this without type punning - I'd be interest in getting a solid answer on whether it should be outright banned or not (I think in C++ it's still a relatively common pattern?).


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:425
+// around.
+#define MAX_DBG_OPS 8
+
----------------
Orlando wrote:
> IIRC there is at least one other place in the compiler where there's a limit for number of debug operands (SCEV or salvaging or both?). Have you considered using a single value for all the limits? Or, if not, that then maybe updating the other limits if they're greater than 8.
I agree, having a global limit would be mostly good - it would also be good to have some good benchmarks for testing different expression lengths, so that we can have an objective indication of what's optimal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128177