Page MenuHomePhabricator

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

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

Details

Reviewers
nikic
jdoerfert
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 used 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.Fri, Jan 8, 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.Sat, Jan 9, 9:58 AM
  • removed more unnecessary braces
jeroen.dobbelaere marked an inline comment as done.Sat, Jan 9, 10:24 AM