[llvm] [RemoveDIs] Add a 'BeforeDbgRecords' parameter to C API isnt insertion functions (PR #92417)
Orlando Cazalet-Hyams via llvm-commits
llvm-commits at lists.llvm.org
Tue May 21 01:52:09 PDT 2024
OCHyams wrote:
> First of all, you wrote a second declaration for `position_before` in `llvm.mli` where I assume you meant to declare `position_before_dbg_records`. Consequently, the tests do not pass.
Oops, I'll fix that.
> Are `LLVMPositionBuilder` and `LLVMPositionBuilder2` intended to co-exist forever, or will the latter eventually deprecate the former?
With the latest changes - intended to co-exist forever.
> If `LLVMPositionBuilder` will be deprecated, I would rather change `position_builder` right now to take the `bool`, and call the underlying C function `LLVMPositionBuilder2` than do the song and dance with the `2`. As far as I know, OCaml libraries don't have these kinds of ABI compatibility concerns, it's much better to rip off the band-aid once, between two major releases, than go through multiple steps of deprecation across multiple major releases.
> Are `position_before` and `position_before_dbg_records` intended to co-exist, or will the latter eventually deprecate the former? By inserting after debug records, does `position_before` have "surprising" behavior that may cause errors?
There is some room for surprising behaviour imo which is why I took my initial approach, however, see @nikic's [comment](https://github.com/llvm/llvm-project/pull/92417#issuecomment-2120078326) - typical api usage probably won't run into it.
Assuming a front end emits instructions in order, most of the time it'll just be inserting at the end of a block or before a terminator. In both these scenarios there's no surprising behaviour and `position_before` does the right thing. It would actually be a mistake to use `position_before_dbg_records` in the case of inserting in order before a terminator, because then all the debug records would end up collecting above the terminator rather than in order of insertion.
If a tool using this api inserts instructions in reverse order by repeatedly setting the "insert before" position then `position_before` would be incorrect - all the debug records would get pushed up to the top of the block. There are other situations where `position_before` is wrong too, which I've described in other comments (e.g. inserting phis).
> If `position_before` has "surprising" behavior, or behavior that shouldn't be the "default," I would rather have it behave as `position_before_dbg_records`, i.e. inserting before debug records be the default action unless the programmer says otherwise.
The `position_before` behaviour is closer to the default in LLVM (LLVM uses a different mechanism to determine "before dbg records", a flag in instruction iterators, and most of the time that is false).
---
I'll wait for @nikic to comment before making further changes - or is it worth opening a discourse discussion at this point?
https://github.com/llvm/llvm-project/pull/92417
More information about the llvm-commits
mailing list