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

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

dexonsmith added inline comments.


================
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");
----------------
dexonsmith wrote:
> 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.`.
> 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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59112