Page MenuHomePhabricator

[InlineFunction] Use llvm.experimental.noalias.scope.decl for noalias arguments.
AcceptedPublic

Authored by jeroen.dobbelaere on Dec 10 2020, 8:30 AM.

Details

Reviewers
nikic
jdoerfert
Summary

Insert a llvm.experimental.noalias.scope.decl intrinsic that identifies where a noalias argument was inlined.

This patch includes some refactorings from D90104.

Diff Detail

Event Timeline

jeroen.dobbelaere requested review of this revision.Dec 10 2020, 8:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2020, 8:30 AM
jeroen.dobbelaere retitled this revision from [InlineFunction] Use llvm.noalias.decl for noalias arguments. to [InlineFunction] Use llvm.experimental.noalias.scope.decl for noalias arguments..
jeroen.dobbelaere edited the summary of this revision. (Show Details)

Rebased and adapted to changes to the intrinsic

nikic added inline comments.Fri, Jan 8, 11:48 AM
llvm/include/llvm/IR/Metadata.h
1216 ↗(On Diff #315132)

This should probably be part of D92887 rather than this one.

llvm/include/llvm/Transforms/Utils/Cloning.h
294 ↗(On Diff #315132)

Is this needed? I originally introduced this class in order to reuse it between inlining and unrolling, but as far as I can tell, you do not make use of it later in the patch series (D92887 implements a separate set of helper functions). If that's right, then I would simply drop these changes and keep the original code.

llvm/include/llvm/Transforms/Utils/Cloning.h
294 ↗(On Diff #315132)

I like the refactoring you did. It also makes it easier to put part of the mechanism early, which is needed (at least in the full restrict case, probably also here) when inlining recursive functions. In that case the caller and callee are the same and gathering things to clone before any change is done was necessary.

I'll move the infrastructure back to InlineFunction as it will indeed only be used there.

  • Update according to comments
  • Keep scope cloning refactoring local
nikic added inline comments.Sun, Jan 17, 9:35 AM
llvm/lib/Transforms/Utils/InlineFunction.cpp
833

The mention of unrolling here no longer makes sense.

868

While this collects the metadata on the experimental.noalias.scope.decl, I'm not seeing any code that would actually update it on the cloned instructions. remap() only checks the noalias and alias.scope metadata, no?

Looking through the tests, I think we're missing a test case that inlines two levels, so that the first creates the intrinsic, and the second has to properly rename its operand.

llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
3

You need to add -aa-pipeline=default to avoid the NPM regression.

Thanks for the review ! I'll try to come with an update later today.

llvm/lib/Transforms/Utils/InlineFunction.cpp
868

Good catch, it indeed seems that I missed the remapping of the 'llvm.experimental.noalias.scope.decl' argument in this version :(

More tests is always better. I'll add one for this, and also one for the caller == callee case.

llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
3

That seems to do the trick.

When working on a test with recursion and inlining, I was thinking about the differences between duplicating scopes in the unrolling (D92887) and duplicating the scopes during the inlining. The refactored code for the inlining keeps the same behavior as before and does a 'deep clone': all the MDNodes and there dependencies are cloned. So the scopes themselves become unique, but also the (function) domains to which they belong are duplicated.

In the utilities added for unrolling (D92887), only the scopes are duplicated. Their domains are kept. And that makes sense as the domain represents the function to which the scope belongs.
The side effect of this, is that the scope layout can be different with different pass orders: loop-unroll after inlining or inlining after loop-unroll will make a difference on how the domains are represented.

IMHO, it would make sense to not do the deep copy for inlining. The scope domain of the inlined function can be kept and shared by multiple functions and multiple inlines, as long as the scopes themselves are unique.
On the other hand, a change in the domain handling can have an effect on the speed of analysis, as an initial filtering is done based on the domains. I don't think we should do this kind of change right now, but it is something we can keep in mind.

Adapted to comments.
Added new testcase for recursive inlining.

jeroen.dobbelaere marked 3 inline comments as done.Tue, Jan 19, 9:13 AM
nikic accepted this revision.Tue, Jan 19, 12:51 PM

LGTM. I would simplify the MetadataAsValue cloning a bit, but it's not particularly important either.

llvm/lib/Transforms/Utils/InlineFunction.cpp
888

These can be dropped (inlining performs only one clone).

919

I don't think it makes sense to separately track MDV and MDVMap. I would create the new MetadataAsValue directly in remap:

if (auto *II = dyn_cast<IntrinsicInst>(I)) {
  if (II->getIntrinsicID() == Intrinsic::experimental_noalias_scope_decl) {
    auto *MV = cast<MetadataAsValue>(
        II->getOperand(Intrinsic::NoAliasScopeDeclScopeArg)));
    auto *NewMV = MetadataAsValue::get(
        I->getContext(), MDMap[cast<MDNode>(MV->getMetadata())]);
    II->setOperand(Intrinsic::NoAliasScopeDeclScopeArg, NewMV);
  }
}

Or so.

1809

The above part of the comment is repeated below. You might want to drop it in one place.

llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll
3

You can drop the --check-prefixes now.

This revision is now accepted and ready to land.Tue, Jan 19, 12:51 PM

Adapted to comments. Simplified the cloning of the MetadataAsValue.

Thanks @nikic for the fast and helpful reviews !

@jdoerfert, could you have an extra look at this one (and the follow ups) ? Thanks !