Page MenuHomePhabricator

[IRBuilder] Set the insert point and debug location together
Needs ReviewPublic

Authored by vsk on Nov 13 2017, 2:46 PM.

Details

Summary

The APIs for setting the insertion point of an IRBuilder behave
inconsistently w.r.t updating the current debug location. The overloads
of SetInsertPoint() may or may not change the current debug location,
and may or may not select a correct location, depending on the selected
overload and the current state of the IRBuilder.

This is inconvenient and causes correctness problems. For more
background on this issue, see the thread:

[llvm-dev] [RFC] Setting the current debug loc when the insertion
point changes

This patch deprecates the following APIs:

SetInsertPoint(BasicBlock *)
SetInsertPoint(Instruction *)
SetInsertPoint(BasicBlock *, BasicBlock::iterator)

They are superseded by the following APIs:

setInsertPoint(BasicBlock *, const DebugLoc &)
setInsertPoint(Instruction *, const DebugLoc &)
setInsertPoint(BasicBlock *, BasicBlock::iterator, const DebugLoc &)
getUnknownDebugLocation(BasicBlock *, BasicBlock::iterator)
getUnknownDebugLocation(Instruction *)

The new APIs offer consistent handling of debug locations by requiring
clients to be more explicit. Uses of the old APIs should be audited and
phased out. New C APIs are available.

Diff Detail

Event Timeline

vsk created this revision.Nov 13 2017, 2:46 PM
vsk updated this revision to Diff 122736.Nov 13 2017, 3:13 PM
  • Upload a diff with context.
aprantl added inline comments.Nov 13 2017, 7:05 PM
include/llvm/IR/IRBuilder.h
139

DebugLoc is already a value type, so we shouldn't pass it by reference.

148

out of curiosity: is there a doxygen command such as \deprecated?

169

either?

192

@{

196

@}

aprantl edited edge metadata.Nov 13 2017, 7:08 PM

Thanks, this is looking really good!

lib/IR/IRBuilder.cpp
25

Is falling back to getCurrentDebugLocation() a good idea? This looks like the kind of magic default behavior that this patch tries to get rid of.

vsk updated this revision to Diff 122772.Nov 13 2017, 8:51 PM
vsk marked 6 inline comments as done.
  • Address review feedback from Adrian.
include/llvm/IR/IRBuilder.h
139

Will fix.

148

Yes, I'll switch to that.

169

Will fix.

lib/IR/IRBuilder.cpp
25

No, it's not a good idea :), and it doesn't match what the comment says it does. Thanks for the catch. I added a check in the unit test which would fail if we used getCurrentDebugLocation() here.

davide added inline comments.Nov 14 2017, 11:21 AM
include/llvm/IR/IRBuilder.h
130

I'd really love to hear Eric (@echristo) input on this, but I have mixed feelings about deprecating an API and introducing a new API with the same name (and just a different overload).
It seems like people might still use this wrong, should we choose a different name?

lib/IR/Core.cpp
2447

The *2 variants are probably OK, and other projects seem to use it successfully.
http://man7.org/linux/man-pages/man2/rename.2.html
Not sure if we have a precedent, but it doesn't seem a bad path forward.

vsk added inline comments.Nov 14 2017, 11:50 AM
include/llvm/IR/IRBuilder.h
130

I'm open to changing the name here to make it more explicit, e.g setInsertPointAndDebugLoc.

lib/IR/Core.cpp
2447

I've seen this pattern floating around, e.g LLVMParseBitcodeInContext2, clang_createTranslationUnit2, etc. IMO '2' implies 'newer', which is good.

echristo edited edge metadata.Nov 14 2017, 1:15 PM

I've made a couple of comments on API naming and deprecating inline.

include/llvm-c/Core.h
2832

I really dislike the "2" moniker on APIs. Seems that if we want a new API that has a new behavior we should do that instead with new and descriptive names.

LLVMPositionBuilderWithLoc?

include/llvm/IR/IRBuilder.h
130

Why are we deprecating a C++ API at all? Just change it.

davide added inline comments.Nov 14 2017, 1:18 PM
lib/IR/Core.cpp
2447

In general these are slightly more counterintuitive to use.
I'd say people shouldn't be really concerned with the past when using an API. I have no strong feelings about it, but maybe it's better to choose a better name.

vsk added inline comments.Nov 14 2017, 1:43 PM
include/llvm-c/Core.h
2832

WithLoc sounds good to me, I'll update it.

include/llvm/IR/IRBuilder.h
130

Deprecating the existing API lets us switch to the new one incrementally, which would be less disruptive. If we don't mind some churn and would rather get this all done in one shot, I'm open to that (& can check in a script). @aprantl, @davide -- thoughts?

aprantl added inline comments.Nov 14 2017, 1:50 PM
include/llvm/IR/IRBuilder.h
130

I don't have a strong opinion. Whatever works best for you. If you are going to convert all uses anyway you might as do it in one go, or you can aim at smaller, incremental commits.

vsk added inline comments.Nov 14 2017, 1:53 PM
include/llvm/IR/IRBuilder.h
130

Well we need to provide an upgrade script anyway. And if we don't remove the old APIs now it's not clear when we will. I'm leaning towards updating everything in one shot.

All at once with an update tool is my preference. :)

Thanks!

-eric

vsk added a comment.Nov 14 2017, 2:43 PM

After some consideration, I no longer think that we should provide an update tool or take the one-shot approach.

Here are the transforms a behavior-preserving update tool would need to make:

IRB.SetInsertPoint(TheBB) => IRB.setInsertPoint(TheBB, IRB.getCurrentDebugLocation())
IRB.SetInsertPoint(TheInst) => IRB.setInsertPoint(TheInst, TheInst->getDebugLoc())
IRB.SetInsertPoint(TheBB, IT) => IRB.setInsertPoint(TheBB, IT, IRB.getStickyDebugLocation(TheBB, IT)) // This one returns IRB.CurrentDbgLoc if TheBB->end() == IT, else IT->getDebugLoc().

A couple points about this:
o The update is fragile because arguments to SetInsertPoint are often potentially side-effecting
o We lose the ability to distinguish between audited and unaudited calls to SetInsertPoint
o We end up adding an API we don't want to promote ('getStickyDebugLocation')

@echristo and others -- in light of this could we go with the incremental route? We can release-note a deadline for upgrading existing calls to SetInsertPoint (say, llvm 6.0?), and I have time to audit these calls.

vsk updated this revision to Diff 122928.Nov 14 2017, 3:21 PM
  • Add a release note, and rename the C APIs so they use 'WithLoc' instead of '2'.
vsk added a comment.Dec 4 2017, 1:09 PM

Friendly ping. I've outlined my thoughts on deprecating the old API versus updating the existing SetInsertPoint users above. Does anyone have feedback about that?

whitequark requested changes to this revision.Dec 6 2017, 8:48 AM
whitequark added inline comments.
include/llvm-c/Core.h
2832

Why should we deprecate these at all? It's legal to pass Loc = NULL for no location, right? Then why shouldn't we make e.g. LLVMPositionBuilderBefore(...) an alias to LLVMPositionBuilderBeforeWithLoc(..., NULL) and keep existing code working? Very few frontends in the wild bother with any debug information at all, I don't think it makes sense to break all of them.

This revision now requires changes to proceed.Dec 6 2017, 8:48 AM
vsk added inline comments.Dec 6 2017, 2:49 PM
include/llvm-c/Core.h
2832

We should deprecate these APIs because they aren't great for building frontends which do emit debug info, and because these APIs will (soon, hopefully) have no C++ counterparts.

It's legal to pass Loc = NULL, but that rewrite is only behavior-preserving for frontends which emit no debug info. To make the transition easier for these frontends, I can provide an upgrade script which rewrites PositionBuilder* to PositionBuilder*WithLoc(..., NULL). Does that sound OK to you?

whitequark added inline comments.Dec 7 2017, 8:44 PM
include/llvm-c/Core.h
2832

No, it does not sound OK because most LLVM frontends aren't written in C, they just use the C interface through FFI. Such a change would be onerous for these frontends for no good reason. I am not convinced that "not having an exact C++ counterpart" is a valid reason for deprecating a C API either.

I would be much more fine with plain out removing SetCurrentDebugLocation since that is used by fewer frontends.

vsk added inline comments.Dec 8 2017, 2:26 PM
include/llvm-c/Core.h
2832

I see only two alternatives to deprecating these C APIs: preserve their behavior or change it s.t they do not forward debug info. There's opposition to a silent behavior change on the mailing list and opposition to deprecating the APIs here.

That leaves the do nothing option: essentially, this patch sans the comments about deprecation. That's what I'll do. I'd be interested in any constructive alternatives you have.

vsk updated this revision to Diff 126212.Dec 8 2017, 2:30 PM
vsk edited edge metadata.
  • Do not mark any C APIs as deprecated.
whitequark added inline comments.Dec 8 2017, 4:15 PM
include/llvm-c/Core.h
2832

The patch looks better to me now. You could still deprecate SetCurrentDebugLocation so that once it is removed, it becomes a hard error for any FFI-based frontend to use it. I don't see any better option.

vsk added a comment.Feb 16 2018, 2:34 PM

Ping, I'd love to find a path forward on this since it's a prerequisite for fixing bugs in LSR and SCEVExpander.

The main remaining issue is what to do with the old IRBuilder APIs for setting the insertion point. We discussed a couple options:

  • I looked at converting all in-tree users to the new API in one shot. We have ~580 users, so this doesn't seem feasible to me without a deprecation period for the old APIs.
  • I started working on an automatic update tool, but found that it's necessarily fragile (e.g when handling side-effecting arguments to API calls). In addition, it would require adding a confusing API to IRBuilder so that clients can mimic the 'sticky' behavior of one of SetInsertPoint's overloads.

Here are some ways I think we can move forward:

  • Rename the new APIs to 'setInsertPointAndLoc' and keep the old APIs in perpetuity.
  • Deprecate the old APIs for 1-2 releases and use the time to migrate in-tree users.

@echristo, @dblaikie, @aprantl and others: I'd like to hear your thoughts on this (especially if you have alternatives not listed here). Thanks!

JDevlieghere accepted this revision.Mar 22 2018, 7:40 AM
In D39982#1010833, @vsk wrote:

Ping, I'd love to find a path forward on this since it's a prerequisite for fixing bugs in LSR and SCEVExpander.
...
Here are some ways I think we can move forward:

  • Rename the new APIs to 'setInsertPointAndLoc' and keep the old APIs in perpetuity.
  • Deprecate the old APIs for 1-2 releases and use the time to migrate in-tree users.

Assuming this is about the C++ APIs, I personally don't see any advantage of your first suggestion and I'd go with the current state of the diff. I also think it doesn't really matter all that much and certainly it's not worth blocking this review over. Either we fix all the in-tree uses by the next release in which case it doesn't matter, or we don't in which case nothing changes for downstream users. Sure, it might be confusing for the transition period, but that's true for both alternatives and at least it's better than what we currently have. Happy to hear other people's opinions if they disagree, but otherwise I'd love to see this getting checked in. As far as I'm concerned this LGTM.