Page MenuHomePhabricator

[Bitcode] Fix bitcode compatibility issue with clang.arc.use intrinsic

Authored by steven_wu on Mar 7 2019, 2:48 PM.



In r349534, objc arc implementation is switched to use intrinsics and at
the same time, clang.arc.use is renamed to llvm.objc.clang.arc.use to
make the naming more consistent. The side-effect of that is llvm no
longer recognize it as intrinsics and codegen external references to
it instead.

Rather than upgrade the old intrinsics name to the new one and wait for
the arc-contract pass to remove it, simply remove it in the bitcode


Diff Detail


Event Timeline

steven_wu created this revision.Mar 7 2019, 2:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 2:48 PM
pete accepted this revision.Mar 7 2019, 5:46 PM


This revision is now accepted and ready to land.Mar 7 2019, 5:46 PM
dexonsmith accepted this revision.Mar 7 2019, 7:29 PM

LGTM too with some nitpicks (forgot to hit "submit" earlier this afternoon).

793–794 ↗(On Diff #189787)

Are we going to grow more intrinsics that don't start with llvm.? I feel like having clang. was a mistake to do, and I wonder if the logic is even worth splitting out like this.

If you still think it's worth splitting, I suggest renaming UpgradeIntrinsicFunction1 while you're in there to something meaningful (maybe in an NFC commit ahead of time), like upgradeLLVMIntrinsicFunction. Then I'd drop the unused parameter from the new one, and call it upgradeClangARCIntrinsicFunction(). This might avoid encouraging new intrinsic names that don't start with llvm..

1585 ↗(On Diff #189787)

I think this deserves a comment for why this is correct or reasonable. Maybe something like: "clang.arc.use is an old name for llvm.arc.clang.arc.use, but we might as well drop it entirely because the optimizer now only recognizes intrinsics for ARC runtime calls."

3 ↗(On Diff #189787)

Can you document what released version of LLVM was used to generate this bitcode file?

dexonsmith added inline comments.Mar 7 2019, 7:42 PM
793–794 ↗(On Diff #189787)

Then I'd drop the unused parameter from the new one

Well, I guess it's not unused, but I'd simplify the logic so it doesn't look like things should be added.

This revision was automatically updated to reflect the committed changes.