[PATCH] D78398: [DebugInfo] Factor out SalvageDebugInfoForDbgValues() from InstCombine

[Chris Jackson via Phabricator via llvm-commits]

chrisjackson added inline comments.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3463
+  SmallVector<std::pair<DbgVariableIntrinsic*, llvm::Value*>, 2> DIIClones;
+  for(auto &User: reverse(DbgUsers))
+  {
----------------
jmorse wrote:
> vsk wrote:
> > @jmorse Is the reverse really needed? Not sure the iteration order in findDbgUsers has any meaning.
> Looks like I mentioned that on the last line of this comment: https://reviews.llvm.org/D56788#1394898
> 
> It's not something that inspires confidence; I think in retrospect that additional salvaging meant additional sinking, giving the test mentioned in that comment an extra dbg.value that isn't tested for. There are three dbg.values now, and only two are tested for. At the time, I probably mistook a new / extra dbg.value as an accidental change in ordering.
> 
> I agree the order in findDbgUsers isn't meaningful, the reverse can go.
I considered removing the reverse but doing so changed the ordering of some output and required a change in test/Transforms/InstCombine/debuginfo_add.ll. I will update the code and test.


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

https://reviews.llvm.org/D78398