This is an archive of the discontinued LLVM Phabricator instance.

[LoopUnroll] Use llvm.experimental.noalias.scope.decl for duplicating noalias metadata as needed
ClosedPublic

Authored by jeroen.dobbelaere on Dec 8 2020, 2:46 PM.

Details

Summary

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=39282. Compared to D90104, this version is based on part of the full restrict patched (D68484) and uses the @llvm.experimental.noalias.scope.decl intrinsic to track the location where !noalias and !alias.scope scopes have been introduced. This allows us to only duplicate the scopes that are really needed.

Notes:

  • it also includes changes and tests from D90104

Diff Detail

Event Timeline

jeroen.dobbelaere requested review of this revision.Dec 8 2020, 2:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2020, 2:46 PM
nikic added a comment.Dec 9 2020, 1:01 AM

I'd suggest to split this up into (at least) three parts:

  • LangRef patch (not present yet here) and the addition of the intrinsic.
  • Teaching everything relevant to look past the intrinsic and emitting it in inliner.
  • Making use of it in loop unrolling.

This will make it easier to identify regressions caused by the extra intrinsic blocking some transform, because of missing special handling or similar.

I'd suggest to split this up into (at least) three parts:

  • LangRef patch (not present yet here) and the addition of the intrinsic.
  • Teaching everything relevant to look past the intrinsic and emitting it in inliner.
  • Making use of it in loop unrolling.

This will make it easier to identify regressions caused by the extra intrinsic blocking some transform, because of missing special handling or similar.

Yes, that makes sense. I hope to have an update tomorrow.

jeroen.dobbelaere retitled this revision from [LoopUnroll] Introduce llvm.noalias.decl and use if for duplicating noalias metadata as needed to [LoopUnroll] Use llvm.noalias.decl for duplicating noalias metadata as needed.
jeroen.dobbelaere edited the summary of this revision. (Show Details)
jeroen.dobbelaere retitled this revision from [LoopUnroll] Use llvm.noalias.decl for duplicating noalias metadata as needed to [LoopUnroll] Use llvm.experimental.noalias.scope.decl for duplicating noalias metadata as needed.
jeroen.dobbelaere edited the summary of this revision. (Show Details)

Rebased and adapted to new naming

nikic added a comment.Jan 8 2021, 12:12 PM

The logic here looks sounds to me.

llvm/include/llvm/Transforms/Utils/Cloning.h
272–274
277

I don't think we use this kind of out_ style anywhere.

llvm/lib/Transforms/Utils/CloneFunction.cpp
933

nit: No braces here.

942

Can operands be null?

954

nit: No braces here.

978

nit: No braces here.

985

nit: auto * or BasicBlock *.

llvm/lib/Transforms/Utils/InlineFunction.cpp
86 ↗(On Diff #315134)

These changes should have probably ended up in D93040.

llvm/lib/Transforms/Utils/LoopUnroll.cpp
591

llvm:: is presumably unnecessary here.

690

With the implementation factored out the "phase 2" and "phase 3" comments look a bit out of place.

693

Possible the ext parameter should be Twine rather than StringRef. But I'm not really familiar with our string API conventions.

llvm/include/llvm/Transforms/Utils/Cloning.h
277

I personally like the the out_/inout_/in_ style for making the purpose clear. But I understand that in the LLVM context it is better to have a common style.

llvm/lib/Transforms/Utils/LoopUnroll.cpp
693

I would rather keep it. When using a Twine, the Twine will be evaluated every time a string is needed.
By using a StringRef, at least that part will only be done once.

  • Update according to comments
jeroen.dobbelaere marked 8 inline comments as done.Jan 9 2021, 9:58 AM
  • removed more unnecessary braces
jeroen.dobbelaere marked an inline comment as done.Jan 9 2021, 10:24 AM

Rebased.
Changed comment to indicate that, while scopes are duplicated, the domain is kept.

frazar added a subscriber: frazar.Jan 19 2021, 1:56 PM
jryans added a subscriber: jryans.Jan 20 2021, 2:32 AM
nikic accepted this revision.Jan 20 2021, 1:17 PM

LGTM

llvm/include/llvm/Transforms/Utils/Cloning.h
271

nit: "back" is too much here.

301

nit: Missing period at end of sentence.

llvm/lib/Transforms/Utils/CloneFunction.cpp
901

nit: Presumably unnecessary llvm:: prefix.

927

I believe you should be able to write this as for (Use &U : I->operands()) and then use U.set() to replace the operand.

930

Should be possible to use something like if (MetadataAsValue *NewMV = ClonedMVScopes.lookup(MV)) here.

937

nit: Should be NeedsReplacement.

942

Here again, something like MDNode *NewMD = ClonedScopes.lookup(MD)) should be possible.

llvm/lib/Transforms/Utils/LoopUnroll.cpp
588

nit: The "Phase1" here no longer makes sense (was dropped below).

llvm/test/Transforms/LoopUnroll/noalias.ll
63

It might be worthwhile to also add a test with noalias.scope.decl outside the loop, that shows that no scopes are cloned in that case.

This revision is now accepted and ready to land.Jan 20 2021, 1:17 PM
nikic added inline comments.Jan 20 2021, 1:35 PM
llvm/test/Transforms/PhaseOrdering/pr39282.ll
18

This TODO should be dropped :)

jeroen.dobbelaere marked 5 inline comments as done.
jeroen.dobbelaere edited the summary of this revision. (Show Details)

Adapted to comments.

jeroen.dobbelaere marked 4 inline comments as done.Jan 21 2021, 5:57 AM

My concern with this patch is in the iteration on all instructions in the blocks in cloneAndAdaptNoAliasScopes and identifyNoAliasScopesToClone.
Perhaps for LoopUnroll is not an issue, if unroll cost model is taken into account before this (i.e. unroll doesn't happen for large BBs). But when doing loop rotate, this may explode for IRs with large BBs, so I'd say some measurements would be helpful here.
For loop rotate, which instructions are cloned from the header to the preheader is known at the callsite, so as to not iterate over the entire original preheader, but I'm not sure this is sufficient.

llvm/test/Transforms/PhaseOrdering/pr39282.ll
57

Nit: CHECKs for MDNodes?

Make use of NoAliasScopeDeclInst

lzutao added a subscriber: lzutao.Jan 24 2021, 4:30 AM
MSxDOS added a subscriber: MSxDOS.Jan 24 2021, 8:21 AM