[clang] [llvm] [IRBuilder] Improve setting of DebugLoc in SetInsertPoint. (PR #147091)
Abid Qadeer via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 18 04:25:58 PDT 2025
abidh wrote:
> I'm not sure I agree with the principle of this change; the current behaviour of not getting a DebugLoc when setting the insertion point to the end of a BasicBlock is long-standing and this change might have unexpected knock-on effects (although if we do make this change, it must also be extended to the `setInsertPoint(BasicBlock*)` version of this function to avoid a confusing difference). I think the more reasonable version of this change would be to save and restore the old debug location explicitly - the RAII version of this pattern, `llvm::InsertPointGuard`, has this behaviour. Updating the `InsertPoint` class and every consumer of its API would be onerous, but has less risk of introducing surprising results; modifying just the code that causes #147063 to save and restore a debug location would be preferable to this, at least imo. I'm open to changing my mind if there's a good argument that this behaviour is correct in principle, however.
Thanks @SLTozer for your comments. I started fixing the individual cases which cause #147063 but then thought that it may be good to fix the problem in more general sense. Although I think the current behavior is not the right one but I do agree that it is long standing one and there may be dependency on this behavior in the clients. On the positive side, the adjustments required in testcases were quite small which indicates that dependency on this behavior may not be very deep (and the debug location in those testcases after this change look better imho).
The rationale for change was that if we are inserting at the end of a BB, the `DebugLoc` of the last instruction in that BB is better match then the `DebugLoc` of a random BB (which is some cases may not even be in the same function). May be the problem is absence of `DebugLoc` in `InsertPoint` which requires us to guess `DebugLoc` to use instead of it being part of `InsertPoint` like it is for `llvm::InsertPointGuard`.
https://github.com/llvm/llvm-project/pull/147091
More information about the cfe-commits
mailing list