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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.

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



================
Comment at: lib/IR/AutoUpgrade.cpp:793-794
   NewFn = nullptr;
-  bool Upgraded = UpgradeIntrinsicFunction1(F, NewFn);
+  bool Upgraded = UpgradeIntrinsicFunction1(F, NewFn) ||
+                  UpgradeIntrinsicFunction2(F, NewFn);
   assert(F != NewFn && "Intrinsic function upgraded to the same function");
----------------
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.`.


================
Comment at: lib/IR/AutoUpgrade.cpp:1585
 
+    // Handle clang.arc.use.
+    if (Name == "clang.arc.use") {
----------------
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."


================
Comment at: test/Bitcode/upgrade-clang-arc-use.ll:3
+
+; RUN: llvm-dis %s.bc -o - | FileCheck %s
+
----------------
Can you document what released version of LLVM was used to generate this bitcode file?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59112/new/

https://reviews.llvm.org/D59112