This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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.Jan 8 2021, 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.Jan 17 2021, 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.Jan 19 2021, 9:13 AM
nikic accepted this revision.Jan 19 2021, 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.Jan 19 2021, 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 !

nikic added a comment.Jan 21 2021, 9:25 AM

Ping @jdoerfert Do you want to check over this patch and the LoopUnroll/LoopRotate ones before they go in?

FWIW, this also lgtm.

I am going to commit this patch today, so that we have a view on possible fallout.

Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2021, 3:12 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I assume this to be a secondary effect of having the instructions in the first place. Maybe some unroll or inline size threshold needs to be thought about them. At the end of the day, we might not be able to avoid something like this as we make !noalias correct, though, I imagine the threshold theory which can be resolved.

I assume this to be a secondary effect of having the instructions in the first place. Maybe some unroll or inline size threshold needs to be thought about them. At the end of the day, we might not be able to avoid something like this as we make !noalias correct, though, I imagine the threshold theory which can be resolved.

The loop unrolling/rotating and the cleanup patches are not yet committed. You can see the effect of those here:

http://llvm-compile-time-tracker.com/compare.php?from=344afa853fcfcc085cb5c957b4a07c7ea013bb1b&to=eaf871f4e7fde26cd755cc4c2d67f2c244c66f18&stat=size-text

Also see D95141 for links to more results.

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

@nikic In the full restrict patches, we also check if the instruction was already handled. I was able to trigger this with an assertion and I have a more or less reduced testcase.

Either we keep a SmallPtrSet and check if the instruction was already handled (this is what the full restrict version does; See D68509 InlineFunction.cpp#969). Or we only replace the metadata if it is in the MDMap (by using MDMap.lookup(M).

Any preference ?

nikic added inline comments.Feb 1 2021, 3:27 AM
llvm/lib/Transforms/Utils/InlineFunction.cpp
929

I don't understand under what circumstances we'd handle an instruction twice.