This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jeroen.dobbelaere on Jan 8 2021, 7:00 AM.

Details

Summary

Similar to D92887, LoopRotation also needs duplicate the noalias scopes when rotating a @llvm.experimental.noalias.scope.decl across a block boundary.
This is based on the version from the Full Restrict paches (D68511).

The problem it fixes also showed up in Transforms/Coroutines/ex5.ll after D93040 (when enabling strict checking with -verify-noalias-scope-decl-dom).

Diff Detail

Event Timeline

jeroen.dobbelaere requested review of this revision.Jan 8 2021, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2021, 7:00 AM
nikic added a comment.Jan 8 2021, 1:12 PM

This is tricker than the unroll case, but also looks sound to me.

llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
482

// if D is part of OrigHeader

483

// if D and U are part of OrigHeader

499

llvm:: probably not needed.

506

Is there any reason to insert a new one before NAD and then move NAD, rather then just inserting the new one at NewHeaderInsertionPoint?

llvm/test/Transforms/LoopRotate/noalias.ll
45

In line with the usual convention, this should be at the start of the @test_02 function.

  • Update according to comments
jeroen.dobbelaere marked 3 inline comments as done.
  • insert clone directly at the right location
jeroen.dobbelaere marked 2 inline comments as done.Jan 9 2021, 10:24 AM
jeroen.dobbelaere added inline comments.
llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
506

hmm. Probably not.. let me change this.

jeroen.dobbelaere marked an inline comment as done.Jan 19 2021, 2:09 PM
jeroen.dobbelaere added inline comments.
llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
415

I'll remove the unnecessary braces in a next version.

nikic accepted this revision.Jan 20 2021, 1:32 PM

LGTM

llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
474

nit: "local restrict declarations" -> "noalias scope declarations"

479

nit: "provenance.noalias" -> "!noalias" or something like that.

515

I think the 1 suffix in these names doesn't make much sense, as there's no 2 etc...

llvm/test/Transforms/LoopRotate/noalias.ll
174

You should be able to drop the TBAA metadata here. It's not used in the test and not relevant for it.

This revision is now accepted and ready to land.Jan 20 2021, 1:32 PM
jeroen.dobbelaere marked an inline comment as done.
jeroen.dobbelaere edited the summary of this revision. (Show Details)

Adapted to comments

jeroen.dobbelaere marked 3 inline comments as done.Jan 21 2021, 6:01 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.

One problem I see is that it is possible that instructions with references to the scopes have been moved before a llvm.experimental.noalias.scope.decl. On the other hand, as far as I see, it should not be 'wrong' to only duplicate the scopes for the instructions that have been moved: at worst, analysis will fall back to 'MayAlias' when comparing a modified to a non-modified version.

I'll try to reduce the instruction range for the OrigPreHeader.

Only adapt instructions in the OrigPreHeader starting from the first new llvm.experimental.noalias.scope.decl. This avoids going over potential very large OrigPreHeader blocks, as mentioned by @asbirlea

Only adapt instructions in the OrigPreHeader starting from the first new llvm.experimental.noalias.scope.decl. This avoids going over potential very large OrigPreHeader blocks, as mentioned by @asbirlea

@nikic, could you take a look at the new changes ? Thanks !

nikic added a comment.Jan 24 2021, 1:50 AM

Still LGTM.

Make use of NoAliasScopeDeclInst