Page MenuHomePhabricator

Add auto-upgrade support for annotation intrinsics
ClosedPublic

Authored by andrew.w.kaylor on Feb 3 2021, 6:22 PM.

Details

Summary

The llvm.ptr.annotation and llvm.var.annotation intrinsics were changedsince the 11.0 release to add an additional parameter. This patch auto-upgrades IR containing the four-parameter versions of these intrinsics, adding a null pointer as the fifth argument.

Diff Detail

Event Timeline

andrew.w.kaylor created this revision.Feb 3 2021, 6:22 PM
andrew.w.kaylor requested review of this revision.Feb 3 2021, 6:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2021, 6:22 PM
aaron.ballman added a subscriber: aaron.ballman.

Adding another potential reviewer.

@pcc CODE_OWNERS.txt says that you own the bitcode. Can you help with this review?

Tyker added a comment.Feb 13 2021, 4:30 AM

This seems good to me but i am not familiar the the auto-upgrader.

Tyker added inline comments.Feb 13 2021, 4:32 AM
llvm/lib/IR/AutoUpgrade.cpp
3759

since the return type of annotation didn't change i am not sure this is necessary.

Tyker added inline comments.Feb 13 2021, 4:33 AM
llvm/lib/IR/AutoUpgrade.cpp
3759

i am not sure the bitcast is necessary.

fhahn added a comment.Feb 17 2021, 9:16 AM

This looks reasonable, but the bit cast creation should be dropped, if it is not necessary.

fhahn requested changes to this revision.Feb 17 2021, 12:25 PM
This revision now requires changes to proceed.Feb 17 2021, 12:25 PM
llvm/lib/IR/AutoUpgrade.cpp
3759

Yes, you're right. I was thinking about the fact that the function type changed, but that doesn't affect the return value type, so the bitcast is not needed.

Removed unnecessary bitcast

I just checked, and it appears the langref still only documents the 4 argument variants (https://llvm.org/docs/LangRef.html#llvm-ptr-annotation-intrinsic). I think those need to be updated as well?

I just checked, and it appears the langref still only documents the 4 argument variants (https://llvm.org/docs/LangRef.html#llvm-ptr-annotation-intrinsic). I think those need to be updated as well?

There's a separate patch for the LangRef here: https://reviews.llvm.org/D96646

fhahn accepted this revision.Feb 22 2021, 1:21 PM

LGTM with the additional comments & assuming passing null is safe.

llvm/lib/IR/AutoUpgrade.cpp
3751

nit: might be good to add a message to the assert, like && "old version of the intrinsic took 4 arguments"

llvm/test/Bitcode/upgrade-ptr-annotation.ll
8

can you pass distinct arguments to the call? Otherwise we are not properly checking if we forward the correct arguments.

llvm/test/Bitcode/upgrade-var-annotation.ll
8

can you pass distinct arguments to the call? Otherwise we are not properly checking if we forward the correct arguments.

This revision is now accepted and ready to land.Feb 22 2021, 1:21 PM

Addressed review feedback