Page MenuHomePhabricator

Handling store to invariant address in LAA
ClosedPublic

Authored by ashutosh.nema on Mar 27 2015, 12:08 AM.

Details

Summary

This change is for handling of “store to invariant address” in ‘LoopAccessAnalysis’(LAA).
While analyzing loop, LAA sets ‘CanVecMem’ to false when store to invariant address appears.

928 void LoopAccessInfo::analyzeLoop(const ValueToValueMap &Strides) {
1029 if (isUniform(Ptr)) {
1030 emitAnalysis(
1031 LoopAccessReport(ST)
1032 << "write to a loop invariant address could not be vectorized");
1033 DEBUG(dbgs() << "LAA: We don't allow storing to uniform addresses\n");
1034 CanVecMem = false;
1035 return;
1036 }

This check was specific to optimizations.

We like to add a flexibility here, by letting optimizations to decide on “store to invariant address”.
Some optimizations may not be interested in “store to invariant address” and few optimization will be interested.

For the same as Adam suggested, made following change:

  1. Added variable ‘StoreToLoopInvariantAddress’ in ‘LoopAccessInfo’ class.
  2. Provided a getter function ‘hasStoreToLoopInvariantAddress’ for ‘StoreToLoopInvariantAddress’.
  3. Updating this variable in ‘LoopAccessInfo::analyzeLoop’ for store to invariant addresses.
  4. In vectorization legality ‘LoopVectorizationLegality::canVectorizeMemory’ checking this variable and returning false in case of store to invariant addresses.

Requesting to review this change.

Diff Detail

Repository
rL LLVM

Event Timeline

ashutosh.nema retitled this revision from to Handling store to invariant address in LAA.
ashutosh.nema updated this object.
ashutosh.nema edited the test plan for this revision. (Show Details)
ashutosh.nema added reviewers: anemet, hfinkel, reames.
ashutosh.nema set the repository for this revision to rL LLVM.
ashutosh.nema added a subscriber: Unknown Object (MLST).
anemet edited edge metadata.Mar 30 2015, 1:27 PM

Please update LAA::print to note if this variable is set and add a test with -analyze in tests/Analysis/LoopAccessAnalysis.

include/llvm/Analysis/LoopAccessAnalysis.h
432–433

it returns true, else returns false. ('s' at the end of return and one sentence or start new sentence with upper case)

475–477

Please initialize to false in the ctor.

lib/Transforms/Vectorize/LoopVectorize.cpp
4010–4012

I think that clang-format likes to indent these further, please check with clang-format-diff.py.

Thanks for review Adam. I'll incorporate your comments.

ashutosh.nema edited the test plan for this revision. (Show Details)
ashutosh.nema edited edge metadata.

Thanks Adam for review.

I have incorporated your comments.

In addition to previous change, this revision contains following:

  1. Updated hasStoreToLoopInvariantAddress comments.
  2. Setting StoreToLoopInvariantAddress to false in constructor.
  3. Corrected formatting.
  4. Added test.

Regards,
Ashutosh

anemet added inline comments.Apr 2 2015, 10:43 PM
lib/Analysis/LoopAccessAnalysis.cpp
1309–1311

"was (not) found" or "has (not) been found"

test/Analysis/LoopAccessAnalysis/store-to-invariant-check1.ll
3 ↗(On Diff #23147)

It's

34 ↗(On Diff #23147)

I may be wrong but I think by default we use no alias analysis (-no-aa), so the tbaa annotations are unnecessary in these two test cases (including the metadata at the end of the files). I would remove them.

Review comments incorporated.
Over previous changes it contains following:

  1. Corrected comments.
  2. Updated print message for invariant store.
  3. Updated test by removing tbaa metadata.
anemet accepted this revision.Apr 6 2015, 5:19 PM
anemet edited edge metadata.

LGTM.

I believe this change is fine because both run-time pointer checking and the dependence analysis are capable to deal with uniform addresses. I.e. it's really just an orthogonal property of the loop that the analysis provides.

Run-time pointer checking will only try to reason about SCEVAddRec pointers or else gives up. If the uniform pointer turns out the be a SCEVAddRec in an outer loop, the run-time checks generated will be correct (start and end bounds would be equal).

In case of the dependence analysis, we work again with SCEVs. When compared against a loop-dependent address of the same underlying object, the difference of the two SCEVs won't be constant. This will result in returning an Unknown dependence for the pair.

When compared against another uniform access, the difference would be constant and we should return the right type of dependence (forward/backward/etc).

This revision is now accepted and ready to land.Apr 6 2015, 5:19 PM

Thanks Adam for review.

I don’t have check-in rights, If possible can you make this check-in ?

Regards,
Ashutosh

anemet added a comment.Apr 7 2015, 2:55 PM

Committed as r234361.

Also somehow you had a few trailing whitespaces. clang-format-diff.py should have been able to catch those, so you may want to double check your workflow.

anemet added inline comments.Apr 7 2015, 9:27 PM
lib/Analysis/LoopAccessAnalysis.cpp
1037

Ashutosh,

I think this should be |= rather than =.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1037
@@ -1046,1 +1036,3 @@
+ // Check for store to loop invariant address.
+ StoreToLoopInvariantAddress = isUniform(Ptr);

// If we did *not* see this pointer before, insert it to  the read-write

Ashutosh,

I think this should be |= rather than =.

Yes, that’s the problem.
I'll fix this, and update you.

Did you reverted 234361 ?

Regards,
Ashutosh

ashutosh.nema edited the test plan for this revision. (Show Details)
ashutosh.nema edited edge metadata.

Over previous change, it contains following:

Fixed: PR23157
StoreToLoopInvariantAddress was not handled correctly and missing previous value.
Added new test case to handle similar scenario.

Looks good, will commit it tomorrow morning. Thanks.

New version landed in r234424.