Page MenuHomePhabricator

[noalias.decl] Look through llvm.experimental.noalias.scope.decl
ClosedPublic

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

Details

Summary

Just like llvm.assume, there are a lot of cases where we can just ignore llvm.experimental.noalias.scope.decl.

Diff Detail

Event Timeline

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

Rebased and updated to the new naming

The vectorizer should handle the 'MetadataAsValue' type as a scalar type.

nikic added a comment.Fri, Jan 8, 9:42 AM

The changes here all look reasonable to me. Ideally we would add test cases for the individual passes and land this before changing inlining, such that the inlining change by itself does not cause undue regressions.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
1751 ↗(On Diff #315336)

This doesn't do anything :)

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2555 ↗(On Diff #315336)

I guess this makes sense with the full version (that accepts an alloca). I don't think this can happen currently.

  • update according to comments
  • prepared for different patch order

FYI: I'll add testcases for the different parts later today

jeroen.dobbelaere edited the summary of this revision. (Show Details)

The guidance for these changes are mostly based on '::assume'. For some changes, I did not find a corresponding (breaking) assume test, so I did not add one for ::experimental_noalias_scope_decl.
For the others I did add testcases.
The change in BasicAliasAnalysis seemed not to be needed to get the testcase working, so I removed the BasicAliasAnalysis changes, but kept the testcase.
For the Vectorization. I currently focused on still allowing vectorization and reducing the amount of intrinsics duplication. It is likely that some finetuning will be needed in future to tackle correctness, but, with this patch, the situation should not be worse that what it is today.

nikic accepted this revision.Sun, Jan 17, 8:24 AM

LGTM

llvm/test/Transforms/EarlyCSE/noalias_scope_decl.ll
3 ↗(On Diff #316993)

across a @llvm.experimental.noalais.scope.decl

14 ↗(On Diff #316993)

same

31 ↗(On Diff #316993)

For this kind of test, using update_test_checks.py with untouched output would be preferred.

This revision is now accepted and ready to land.Sun, Jan 17, 8:24 AM

Adapted to comments

jeroen.dobbelaere marked an inline comment as done.

Oops. Adapted to comments (but now for real).

jeroen.dobbelaere marked 2 inline comments as done.Tue, Jan 19, 9:12 AM