[llvm] [DebugInfo][LICM] Fix missing debug location updates (PR #91729)
Orlando Cazalet-Hyams via llvm-commits
llvm-commits at lists.llvm.org
Fri May 10 07:44:57 PDT 2024
https://github.com/OCHyams commented:
Thanks for another patch. Code and test SGTM, but there are a couple of questions first.
> So, it's debug location would be dropped immediately in the function hoist(), and I didn't create a test for it. However, IMO, assigning a proper debug location for a new instruction is reasonable and appropriate, though the assigned debug location would be manipulated afterwards. I'd like to take any suggestion on this special case.
This is an interesting observation, that `hoist` calls `updateLocationAfterHoist` which calls `dropLocation`.
I think there's a future in which we preserve more location information even after hoists - this is an area we're actively looking into right now (cc @SLTozer).
Given that, I would actually be in favour of doing the location propagation as you have done, and also adding an assert after the `hoist` which check that the location has been dropped (I think `assert(!ReciprocalDivisor.getDebugLoc())` should work? ), with a comment pointing out that if the assert fires a test case should be added and it's safe to remove the assert.
This may end up being an unpopular opinion (propagating the location is a noop because it'll get dropped, but it still has a runtime cost). @pogo59 wdyt? I might be making it up but I think I recall a similar discussion happening recently where you were involved.
https://github.com/llvm/llvm-project/pull/91729
More information about the llvm-commits
mailing list