[PATCH] D61198: [IRBuilder][DebugInfo] Don't pick DebugLocs for new instructions from debug intrinsics

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: include/llvm/IR/IRBuilder.h:139
+    // Avoid taking DebugLocs from debug intrinsics
+    auto DbgSrcInst = skipDebugIntrinsics(InsertPt);
+    SetCurrentDebugLocation(DbgSrcInst->getDebugLoc());
----------------
This could end up returning the end() iterator (so the assert above is now out-of-play).


================
Comment at: include/llvm/IR/IRBuilder.h:150
+      // Avoid taking DebugLocs from debug intrinsics
+      auto DbgSrcInst = skipDebugIntrinsics(InsertPt);
+      SetCurrentDebugLocation(DbgSrcInst->getDebugLoc());
----------------
You probably want to do this before checking for end() (to avoid reading debug loc from end()).

Another interesting thing here is that when setting the insertion point to the end of the BB (or rather after the last non-debug-intrinsic in the BB) the current debug location won't be updated. Is some user of this method relying on that. Should perhaps the current debug location be invalidated instead?

My thoughts are:
- Should we simply skip setting debug location when insertion point is a debug intrinsic?
- Or should we invalidate the debug location when insertion point is a debug intrinsic?
- Should perhaps these SetInsertPoint methods take a second argument, providing the debug location to set (only ~800 call sites to update...)?
- Or should we never do the side effect of updating debug location (just as many call sites to potentially update with explicit settings)? Notice that the `SetInsertPoint(BasicBlock *TheBB)` version does not set the current debug location. So at the moment it isn't obvious that SetInsertPoint always updates the current debug location.

Haven't really thought about what I'd prefer myself.


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

https://reviews.llvm.org/D61198