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

Authored by vsk on Mon, Nov 13, 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

vsk created this revision.Mon, Nov 13, 2:46 PM
vsk updated this revision to Diff 122736.Mon, Nov 13, 3:13 PM
  • Upload a diff with context.
aprantl added inline comments.Mon, Nov 13, 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

@}

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.Mon, Nov 13, 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.Tue, Nov 14, 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.Tue, Nov 14, 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.

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.Tue, Nov 14, 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.Tue, Nov 14, 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.Tue, Nov 14, 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.Tue, Nov 14, 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.Tue, Nov 14, 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.Tue, Nov 14, 3:21 PM
  • Add a release note, and rename the C APIs so they use 'WithLoc' instead of '2'.