This is an archive of the discontinued LLVM Phabricator instance.

Remangle intrinsics names when types are renamed
ClosedPublic

Authored by apilipenko on Apr 21 2016, 10:31 AM.

Details

Summary

This is a fix for the problem mentioned in "LTO and intrinsics mangling" llvm-dev mail thread:
http://lists.llvm.org/pipermail/llvm-dev/2016-April/098387.html

Two approaches to fix the problem were mentioned: remangle intrinsics names when types are renamed or avoid using pointer other than i8* in masked load/store intrinsics. I prototyped both and decided to go ahead with the former. It's just cleaner and simple than shuffling masked load/store arguments in autoupgrade/builder/tests. It's also more generic and I expect that we will have the same problem with gather/scatter when we decide to support arbitrary address spaces in these intrinsics.

In this patch I reuse AutoUpgrade to trigger intrinsic renaming in the case when types were renamed during module loading. I also changed IRMover to rename the intrinsics if the types were remapped.

Diff Detail

Event Timeline

apilipenko retitled this revision from to Remangle intrinsics names when types are renamed.
apilipenko updated this object.
apilipenko added a subscriber: llvm-commits.
mehdi_amini requested changes to this revision.Apr 21 2016, 10:47 AM
mehdi_amini edited edge metadata.

I have a strong feeling against any auto upgrade of the bitcode produced by the current version of llvm

This revision now requires changes to proceed.Apr 21 2016, 10:47 AM

Let me clarify: I'm not opposed about doing the renaming during bitcode loading, I just think it is wrong to use the autoupdate path.
I'd expect that it could be done at the time you parse the intrinsic from the bitcode?

Potentially renaming can be needed for textual representation as well. So this functionality has to be shared between parsers. I can do something similar to AutoUpgrade for this specific scenario, but it will duplicate AutoUpgrade logic about iterating over declarations/call sites.

apilipenko updated this revision to Diff 54660.Apr 22 2016, 8:36 AM
apilipenko edited edge metadata.

Don't use AutoUpgrade anymore. I'm going to add tests as soon as we agreed on the functional part.

mehdi_amini added inline comments.Apr 22 2016, 10:07 AM
lib/AsmParser/LLParser.cpp
224

RAUW does not work at this point?

lib/IR/Function.cpp
1050

naming

1054

Guess you didn't want it here.

1069

std::all_of

1079

There are two conceptually separated path IIUC:

  1. old bitcode the intrinsic is *not* mangled and need to be mangled. This is the auto-upgrade path.
  2. current bitcode has named type collision in the context and you need to remangle.

The fact that the remangling is shared is fine. However the conditions to reach this should be asserted better: the first path is valid only if the intrinsic was not mangled in the first place. The second path is valid only if the mismatch is because of a suffix added to the type.

apilipenko added inline comments.Apr 25 2016, 5:23 AM
lib/AsmParser/LLParser.cpp
224

It works, autoupgrade uses it, but what are the benefits over changing the existing call instruction?

lib/IR/Function.cpp
1079

In the first "not mangled intrinsic" path do you mean masked load/store intrinsics specifically?

In general, first I try to upgrade an intrinsic (see BitcodeReader.cpp:3220). If it was upgraded I assume that the name and the signature matches and no further remangling is needed. If it wasn't upgraded I check if it needs remangling. We can only remangle intrinsics which signature matches with the .td descriptor, that's why I do upgrade first.

With masked load/store autoupgrade patch applied in case of renamed types autoupgrade bit kicks in first and remangles the intrinsic. It happens because autoupgrade path also matches the name with the expected name for the given signature. It makes this case slightly convoluted but I don't see any other simple way to check if autoupgrade is needed.

Since we know the old mangling scheme for the masked intrinsics we might try to match the name with the old mangling name.

if (Name == Intrinsics::getName(masked_load, OldTys)
  // Upgrade

But it won't work in case when types were renamed.

Difficulties separating upgrade and remangling paths was one of the reasons I combined them both in autoupgrade in the initial revision.

reames edited edge metadata.May 5 2016, 5:49 PM

Minor comments. Note that there are open questions here which I'm not qualified to answer. joker.eph?

lib/AsmParser/LLParser.cpp
223

You can also invoke some intrinsics. Use CallSite or handle InvokeInst.

224

RAUW is idiomtic. Please use it unless there's a reason not to.

lib/Bitcode/Reader/BitcodeReader.cpp
3419

Could this be pushed inside upgradeIntrinsicFunction?

5579

Overly complicated. Can this be expressed as a single loop which does a then b?

lib/IR/Function.cpp
1069

or for (auto *P : FTy->params())

@reames, your suggestion to move the logic to upgradeIntrinsicFunction is basically what I had in the first diff of the review. But there was an argument to avoid using autoupgarde for cases which are technically not upgrade, i.e. for the IR generated by the same version of LLVM.

lib/AsmParser/LLParser.cpp
223

Good catch, will fix.

apilipenko updated this revision to Diff 58615.May 26 2016, 8:22 AM
apilipenko edited edge metadata.

Take some of the Philip's comments into account: add handling for invokes and use for-each loop in remangleIntrinsicFunction.

Tests for all theses cases?

lib/AsmParser/LLParser.cpp
224

RAUW?

lib/Bitcode/Reader/BitcodeReader.cpp
5587

Why do we update only calls and not all uses?
If we don't expect any other use of an intrinsic than calls, then we should just cast.

lib/IR/Function.cpp
1050

ping (upper case for variable)

Address review comments, tests added.

apilipenko marked 3 inline comments as done.Jun 17 2016, 11:25 AM
mehdi_amini accepted this revision.Jun 17 2016, 11:30 AM
mehdi_amini edited edge metadata.

LGTM, thks.

This revision is now accepted and ready to land.Jun 17 2016, 11:30 AM
This revision was automatically updated to reflect the committed changes.