This is an archive of the discontinued LLVM Phabricator instance.

[LAA] Add Memory dependence remarks.
ClosedPublic

Authored by malharJ on Aug 19 2021, 6:11 AM.

Details

Summary

Adds new optimization remarks when vectorization fails.

More specifically, new remarks are added for following 4 cases:

  • Backward dependency
  • Backward dependency that prevents Store-to-load forwarding
  • Forward dependency that prevents Store-to-load forwarding
  • Unknown dependency

It is important to note that only one of the sources
of failures (to vectorize) is reported by the remarks.
This source of failure may not be first in program order.

A regression test has been added to test the following cases:

a) Loop can be vectorized: No optimization remark is emitted
b) Loop can not be vectorized: In this case an optimization
remark will be emitted for one source of failure.

Diff Detail

Unit TestsFailed

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
llvm/test/Transforms/LoopVectorize/loopvectorize-opt-remarks.ll
27 ↗(On Diff #368011)

Your tests all use the same types, I believe tbaa is not how the aliasing issues are detected, and you can remove those nodes

37 ↗(On Diff #368011)

I am not sure i understand this test. The description says the loop contains only reads, but the IR has stores in it.
Also, the IR is already vectorized, so it's not really a useful test case

179 ↗(On Diff #368011)

same here, the IR is already vectorized. For such a patch i would have expected all loops to be scalar, as before entering the Loop Vectorizer

13 ↗(On Diff #367580)

I am not aware of any tool to do this cleanup

malharJ updated this revision to Diff 368353.Aug 24 2021, 8:21 AM
malharJ marked 4 inline comments as done.
  1. Added a standardized method of outputting the debug location.

Also changed the output format accordingly.

  1. Simplified the regression test:
    • Corrected bug by changing vectorized IR to scalar IR
    • removed as much debug info as possible
llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
960–966 ↗(On Diff #367580)

Ok, I've used something more standardized now to print out the debug location.

llvm/test/Transforms/LoopVectorize/loopvectorize-opt-remarks.ll
37 ↗(On Diff #368011)

My bad here.
Updated the patch with the scalar version for the three cases (nodep, forward, backwardVectorizable)

Thanks Malhar. Code looks good to me. I'll need to take another look at the unit tests.
I'm going away for a week, so either someone else picks up the review in the meantime, or we can resume working on this when I come back.

malharJ added a comment.EditedAug 31 2021, 12:15 AM

ping.

(Can someone else please review the patch as well ?)

fhahn added a comment.Sep 1 2021, 4:51 AM

Rather than interpreting the LoopAccessAnalysis result in LV, can LAA directly generate the reports? Then other users of LAA could also benefit.

@fhahn , but these remarks are specific to vectorization (ie. scenarios where loop does not get vectorized) ...
are there any passes (other than LV) which would require this information ?

david-arm added a comment.EditedSep 3 2021, 2:04 AM

@fhahn , but these remarks are specific to vectorization (ie. scenarios where loop does not get vectorized) ...
are there any passes (other than LV) which would require this information ?

Hi @malharJ, lots of passes actually use LoopAccessAnalysis, not just LoopVectorize.cpp, i.e. LoopLoadElimination.cpp, LoopVersioningLICM.cpp, etc. I agree they aren't strictly concerned about vectorisation, but they will execute the same code paths, i.e. LoopAccessInfo::analyzeLoop -> MemoryDepChecker::areDepsSafe. Maybe it does make sense for elaborateMemoryReport to live in LoopAccessAnalysis.cpp, especially since all the data used to generate the report actually lives in LoopAccessInfo anyway?

llvm/lib/Analysis/LoopAccessAnalysis.cpp
589

Remark instead of 'insight'?

1764

Remark instead of 'insight'?

malharJ updated this revision to Diff 370670.Sep 3 2021, 1:59 PM
malharJ marked 2 inline comments as done.
  • Moved remark generating function (elaborateMemoryReport()) and some related functions from LoopVectorizationLegality to the class LoopAccessInfo (inside the file LoopAccessAnalysis).
  • Deleted a redundant function createMissedAnalysis() and replaced it's usage with that of recordAnalysis().
  • elaborateMemoryReport() could not be placed within CanVectorizeMemory() as the latter is a const function and recordAnalysis() (which is used by elaborateMemoryReport()) cannot be a const function as it modifies a member variable (Report)

    Hence elaborateMemoryReport was placed after the call to analyzeLoop().
  • A direct consequence of moving the reporting code to LoopAccessAnalysis meant that some memory dependence related remarks from earlier became redundant. These have been removed since elaborateMemoryReport() will take care of such remarks.
  • Moving code to LoopAccessAnalysis meant that it would make more sense for the remarks are to be emitted when "-Rpass-analysis=loop-accesses" is used (and not "-Rpass-analysis=loop-vectorize")

    Hence the LIT test is also updated to reflect the same.

I quite like the way it looks with the code moved from the vectorizer to loop access analysis. I'll do a more in-depth review after the unit tests are fixed, but i left a couple of simple comments for now.

llvm/lib/Analysis/LoopAccessAnalysis.cpp
2135

why was this remark removed? As far as i can see, this message is not covered by the new memory report

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
889 ↗(On Diff #370670)

these braces can now be removed

fhahn added a comment.Sep 6 2021, 4:57 AM

@fhahn , but these remarks are specific to vectorization (ie. scenarios where loop does not get vectorized) ...
are there any passes (other than LV) which would require this information ?

Thanks for the update. As David already said, other passes also use LAA and also there's nothing vectorisation specific about the remark you added.

llvm/include/llvm/Analysis/LoopAccessAnalysis.h
250

There's no need to return a SmallVector with the length encoded, the callers should not care about that. You can use ArrayRef if the callers do not add to the vector or SmalLVectorImpl if they need to add elements.

llvm/lib/Analysis/LoopAccessAnalysis.cpp
589

Use /// for all comments. Also, does this need to be public after the recent changes?

1759

Why call it UnsafeDependences if it is supposed to only contain unknown dependences?

llvm/test/Transforms/LoopVectorize/loopvectorize-opt-remarks.ll
1 ↗(On Diff #370670)

please avoid adding tests with -enable-new-pm=0. You should be able to move that one to the LoopAccessAnalysis directory.

6 ↗(On Diff #370670)

Instead of having the C code here, I think it would be more helpful if you explain what this tests in a sentence or 2 with references to the IR, e.g. which memory instruction/GEP has the uncomputable bound and *why*.

malharJ updated this revision to Diff 370983.EditedSep 6 2021, 7:25 PM
malharJ marked 4 inline comments as done.
  1. Fixed the unit tests:

    This required initializing the ORE object using one of the constructors: OptimizationRemarkEmitter(const Function& F)

    Previously several of the unit tests were hitting memory errors and they seem to have been fixed by this change.

    There are also some minor formatting updates to the remark emitted.
  1. Removed checks for loop distribution pragma remark from LIT tests. This is because there is no analysis being done to support it.
  1. Updated recordAnalysis() to allow generating multiple reports when they are of the same type. This seems like a logical thing to do, for example if you have multiple instances of unbound array index case in a loop, it would be good for a user to view (in the report) similar types of errors and then correct them.
  1. Updated LIT test to use new Pass Manager syntax
  1. Minor formatting updates
malharJ added inline comments.Sep 6 2021, 7:27 PM
llvm/include/llvm/Analysis/LoopAccessAnalysis.h
250

Currently the code uses "auto" when accessing the. value returned by this getter
so there isn't a need for the user to know the size used in the template ...

regardless, I've changed it to use SmallVector<T> instead if that's ok.

llvm/lib/Analysis/LoopAccessAnalysis.cpp
589

It was being accessed from outside the class on line 2045.

I've moved it to private now and added a public getter instead.

1759

I think UnsafeDependences contains both unknown and known unsafe dependences.
I think the comment is unclear so I'm removing it.

malharJ updated this revision to Diff 370992.Sep 6 2021, 11:08 PM

This is to correct a mistake made in the previous patch.

"loop not vectorized" should not be emitted from loop-access analysis.
(the loop-vectorize takes care of adding 'loop not vectorized')

llvm/test/Transforms/LoopVectorize/diag-with-hotness-info-2.ll
25

This hint is a useful one, and it was added purposefully. It should remain unchanged. I would suggest that you add a case in elaborateMemoryReport to recreate this hint.

malharJ updated this revision to Diff 371057.Sep 7 2021, 6:19 AM
malharJ marked an inline comment as done.

Re-added the remark/note about using pragma loop distribute.

Just one small comment. Otherwise the patch looks good to me

llvm/include/llvm/Analysis/LoopAccessAnalysis.h
524

Nit: Missing a slash

malharJ updated this revision to Diff 371371.Sep 8 2021, 9:28 AM
malharJ marked 2 inline comments as done.

Trivial formatting update.

Thanks Malhar. The patch looks good to me

sdesmalen added inline comments.Sep 9 2021, 5:04 AM
llvm/include/llvm/Analysis/LoopAccessAnalysis.h
250

If there is no need to modify the array returned by getUnsafeDependences (which there isn't, because the returned value is const), ArrayRef seems the better class to use, because it has all sorts of convenient utilities and is similarly just a (immutable) reference to the array.

llvm/lib/Analysis/LoopAccessAnalysis.cpp
2114

Is this case not already covered by the recordAnalysis call above?

It seems like a mechanism for doing this already exists (recordAnalysis), it may be worth extending that to handle multiple reports (there is currently a limitation that it uses a single Report variable, but perhaps that could be made into a vector of reports which can be appended to).

2158

nit: single-use variable, can be inlined in the next statement.

2215–2216

nit: please start your comments with capitalisation and end with a period.

llvm/test/Analysis/LoopAccessAnalysis/memory-dep-remarks.ll
12 ↗(On Diff #371371)

Would a prefix like "Loop cannot be vectorized: " be useful?

I think the message is a bit cryptic, how about:

Unable to determine aliasing information because the bounds of this access cannot be computed.
malharJ updated this revision to Diff 371619.Sep 9 2021, 9:13 AM
malharJ marked 3 inline comments as done.
  • Updated remark for unknown array bounds case
  • Moved call to recordAnalysis() inside elaborateMemoryReport() for the UnsafeDataDependenceTriedRT case
  • Changed getUnsafeDependences() to return an ArrayRef since return value is only read.
  • Minor formatting updates
malharJ added inline comments.Sep 9 2021, 9:14 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
2114

I agree it has been covered by the call to recordAnalysis() at line 2070 ...
I've now moved that call to recordAnalysis() to inside elaborateMemoryReport().

Regarding the second issue, I'm not sure myself why recordAnalysis has Report as scalar and not a vector but that is not really a part of my change ... I just tried to re-use it.

llvm/test/Analysis/LoopAccessAnalysis/memory-dep-remarks.ll
12 ↗(On Diff #371371)

If you see the earlier version/diffs of the patch, the reporting was being done in LoopVectorizer.
That time I had added "loop not vectorized" in the tests.

But now that it has moved to LoopAccessAnalysis, we are no longer emitting "loop not vectorized" here.
That prefix is prepended by the function reportVectorizationFailure() in LoopVectorize.cpp

For the latter part, I have made the change.

malharJ updated this revision to Diff 371751.Sep 9 2021, 4:59 PM

corrected trivial typo in LIT test.

malharJ updated this revision to Diff 371826.Sep 10 2021, 1:49 AM

minor formatting update for fixing a failing test.

Are there any other review comments ?

david-arm accepted this revision.Oct 1 2021, 12:29 AM

LGTM! It looks like you've addressed all the other reviewer's comments and I think the patch looks good now. Thanks!

llvm/lib/Analysis/LoopAccessAnalysis.cpp
2219

nit: Maybe it's worth moving the recordAnalysis call to before the for loop, i.e.

OptimizationRemarkAnalysis R = recordAnalysis("UnknownArrayBounds", I);
for (...)
This revision is now accepted and ready to land.Oct 1 2021, 12:29 AM
fhahn added inline comments.Oct 1 2021, 1:53 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
795

If computing the bounds fails here, we may retry creating checks by adding assumptions below (see line 806).

I think it could happen that we have multiple uncomputable pointer bounds here, but for some of them we may be able to actually compute bounds below. Should we remove the ones we can compute bounds below from the set?

llvm/test/Analysis/LoopAccessAnalysis/memory-dep-remarks.ll
12 ↗(On Diff #371371)

Unable to determine aliasing information because the bounds of this access cannot be computed.

I'm not sure this is entirely accurate. AFAIK uncomputable bounds here mean we cannot generate runtime checks.

'aliasing information' seems ambiguous here, because at the point where we check whether the bounds are computable, we already determined that the access may alias another access, independent of whether the bounds can be computed or not (like the underlying objects may alias).

malharJ updated this revision to Diff 380621.Oct 19 2021, 2:38 AM
malharJ marked an inline comment as done.
  1. Updated format of remark
  2. Removed pointers from UncomputablePtrs Set if it's bound can be computed on a retry (with Assumptions).
malharJ added inline comments.Oct 19 2021, 2:39 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
795

Done. thanks for that suggestion.

2219

Unfortunately that can't be done because Instruction* I is being declared inside the for-loop.

llvm/test/Analysis/LoopAccessAnalysis/memory-dep-remarks.ll
12 ↗(On Diff #371371)

Ok I agree.

I have changed the remark accordingly.

Are there any further review comments ?

malharJ updated this revision to Diff 383842.Nov 1 2021, 11:13 AM
  • Removed the mechanism that collected the remarks (enum "FailureReason" and function "elaborateMemoryReport()") and emitted them at the end.

    Instead, now the remarks are emitted inside LoopAccessInfo::analyzeLoop(), ie. while the loop is being analyzed.
  • Only one remark is emitted, at the point it is found.
malharJ updated this revision to Diff 384022.EditedNov 2 2021, 3:08 AM
  • Removed remark emission (OptimizationRemarkEmitter) to outside LoopAccessAnalysis. LAA generates a "Report" which can be used by other passes to emit remarks.

This also fixes the issue with emission of the same/similar remark twice (once by LAA, and once by a parent pass, eg: LV)

  • Updated the tests accordingly.
malharJ updated this revision to Diff 384037.EditedNov 2 2021, 4:10 AM

Fixed minor typos and one LIT test (vectorization-remarks-missed.ll)

@sdesmalen , can you please review the latest updates ?

sdesmalen requested changes to this revision.Nov 10 2021, 5:32 AM

Hi @malharJ, thanks for cleaning up the patch and making changes based on what we discussed offline, it's a bit more manageable to review now.
There are still some changes that I think are unnecessary, and I found that the patch needs to be rebased (this is how I found some confusing output test/Analysis/LoopAccessAnalysis/pointer-phis.ll).

llvm/include/llvm/Analysis/LoopAccessAnalysis.h
618

This seems unused?

llvm/lib/Analysis/LoopAccessAnalysis.cpp
2083

UncomputablePtr and the mechanism around it seems entirely redundant, because it doesn't add any information that is used anywhere. The only reason that UncomputablePtr is collected is to avoid printing "Cannot identify array bounds" when it gets to the then-block of if (!CanDoRTIfNeeded) { ... }. The information is redundant, because when UncomputablePtr is set, then CanDoRT is false, and vice-versa.

2086

nit: The capitalisation of cannot -> Cannot seems unnecessary.

sdesmalen added inline comments.Nov 10 2021, 5:32 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
1762

Can there only be a single unsafe/unknown dependence? Or can there be more?

2114

unnecessary change.

2140

These changes here obfuscate the report that's generated by LAA when running loop(print-access-info). The information printed was:

Loop access info in function 'store_with_pointer_phi_incoming_phi':
  loop.header:
 The compiler can't determine the cause of the issue.
    Dependences:
      Unknown:
          %v8 = load double, double* %arrayidx, align 8 ->
          store double %mul16, double* %ptr.2, align 8

The information that is added by the remark is:

Loop access info in function 'store_with_pointer_phi_incoming_phi':
  loop.header:
    Report: unsafe dependent memory operations in loop. Use #pragma loop distribute(enable) to allow loop distribution to attempt to isolate the offending operations into a separate loop
Unknown data dependence. Memory location is the same as accessed at <UNKNOWN LOCATION>. 
 The compiler can't determine the cause of the issue.
    Dependences:
      Unknown:
          %v8 = load double, double* %arrayidx, align 8 ->  
          store double %mul16, double* %ptr.2, align 8

The extra information here isn't particularly useful, especially because it doesn't have any line information. Perhaps the code should disable the extra info if the location is unknown/unavailable.

2142

nit: Please remove the newline.

2142

Instead of using + to concatenate two strings, let this be handled by raw_ostream using << .

llvm/test/Analysis/LoopAccessAnalysis/stride-access-dependence.ll
121

Please CHECK-NEXT for the full line that is printed.

This revision now requires changes to proceed.Nov 10 2021, 5:32 AM
malharJ updated this revision to Diff 387087.Nov 14 2021, 7:44 AM
malharJ marked 6 inline comments as done.
  • Rebased patch onto main
  • Updated code to avoid printing <unknown> when no debug info is present
  • minor formatting updates
llvm/include/llvm/Analysis/LoopAccessAnalysis.h
618

thanks for pointing it out. Removed it.

llvm/lib/Analysis/LoopAccessAnalysis.cpp
1762

There can be more.

But we are only emitting (as a remark) the first one found.

2083

Ok, I agree but there's an issue here ..

LoopAccessInfo::recordAnalysis() cant be called from within the function ( AccessAnalysis::canCheckPtrAtRT() ).
as it's not a member of AccessAnalysis class.

Perhaps one way to do it would be for AccessAnalysis::canCheckPtrAtRT() to accept a parameter
by reference and then we can use the value after the call as input to recordAnalysis() here.
But I'm afraid this approach is not much less verbose than current approach pf having UncomputablePtr as a member variable.

2140

Thanks for pointing this out. Done.

2142

can we keep this newline ?

The current text (Note: it was not introduced by this patch) is too long already:

unsafe dependent memory operations in loop. Use "#pragma loop distribute(enable) to allow loop distribution to attempt to isolate the offending operations into a separate loop

llvm/test/Analysis/LoopAccessAnalysis/stride-access-dependence.ll
121

Agreed.

But now the code will only print the remaining text: "Memory location is the same as ...etc"
when debug info is available.

so this is no longer an issue (since this test does not contain any debug info/metadata).

sdesmalen added inline comments.Nov 24 2021, 9:16 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
587

UncomputablePtr doesn't really need a separate variable in AccessAnalysis, since it's only set by one function, and used directly by the function that calls it.

You can change canCheckPtrAtRT to take Value **UncomputablePtr = nullptr, and in canCheckPtrAtRT assign the value if the value is requested:

if (UncomputablePtr)
  *UncomputablePtr = Access.getPointer();

Where you call it, you can have:

Value *UncomputablePtr = nullptr;
bool CanDoRTIfNeeded =
    Accesses.canCheckPtrAtRT(*PtrRtChecking, PSE->getSE(), TheLoop,
                             SymbolicStrides, false, &UncomputablePtr);

if (!CanDoRTIfNeeded) {
  if (auto *I = dyn_cast_or_null<Instruction>(UncomputablePtr))
    recordAnalysis("UnknownArrayBounds", I)
}

Can you split this change out into a separate patch with a test that demonstrates the change?

1765

It shows here that the dependences are already collected. You can iterate Dependences to find the dependence that isn't safe, so that you don't have to add UnsafeDependence and maintain that separately.

1847

I guess this can be more briefly written as:

Loc = I->getDebugLoc();
if (auto *PtrI = dyn_cast_or_null<Instruction>(getPointerOperand(I)))
  Loc = PtrI->getDebugLoc();

(at which point it probably doesn't require a separate function anymore, especially since it has only one use).

1850–1852

This seems like a case that should be added to getPointerOperand ?

2084

It now passes in I as the instruction, but the debug location of the remark seems unchanged in the test. What value is this adding?

2085

Just leave the name as CantIdentifyArrayBounds ?

RKSimon resigned from this revision.Nov 29 2021, 2:50 AM

Sorry this is not an area I known enough about to review

fhahn added inline comments.Nov 29 2021, 3:20 AM
llvm/include/llvm/Analysis/LoopAccessAnalysis.h
298

Does this need to be a shared_ptr? If you want to encode the fact that it may not be set, using Optional may be a better choice. Or you could initialize it to NoDep in case there is no unsafe dependence.

malharJ updated this revision to Diff 394825.EditedDec 16 2021, 4:42 AM
malharJ marked 4 inline comments as done.
  • Moved the changes for testing unknown array bounds to new patch: D115873
  • addressed review comments
malharJ retitled this revision from [LAA] Add Memory dependence and unknown bounds remarks. to [LAA] Add Memory dependence remarks..Dec 16 2021, 4:58 AM
malharJ edited the summary of this revision. (Show Details)
malharJ marked an inline comment as done.Dec 16 2021, 5:19 AM
malharJ added inline comments.
llvm/include/llvm/Analysis/LoopAccessAnalysis.h
298

I've removed it now based on review comment by sdesmalen, so this is no longer an issue.

llvm/lib/Analysis/LoopAccessAnalysis.cpp
587
1765

Done.
That's a good point, thanks !

1847

I've removed this function and inlined the code since there is only one usage.

2084

This was essentially an issue in the original test,
The debug info (!35) used by the loop was incorrect.
I've changed it to use !32 now.

I have corrected the issue in my new patch:
https://reviews.llvm.org/D115873

malharJ updated this revision to Diff 394853.EditedDec 16 2021, 6:42 AM
malharJ marked an inline comment as done.
  • rebased patch

Premerge îs failing due to (a possible bug introduced by) recent changes:
https://github.com/google/llvm-premerge-checks/pull/374

malharJ updated this revision to Diff 394998.Dec 16 2021, 2:24 PM
  • Updated unit test
fhahn added inline comments.Dec 28 2021, 9:53 AM
llvm/include/llvm/Analysis/LoopAccessAnalysis.h
19

This change is unnecessary, keep the include in the .cpp

llvm/lib/Analysis/LoopAccessAnalysis.cpp
2135

It seems like it would make things easier to read if the logic would be in a separate function, with proper documentation what it is supposed to do?

2137

nit: variable names here start with upper cases

2140

nit: dependence instead of dependency, to be in line with the terminology used elsewhere in the file?

2148

Is this true? Unless I miss something, this emits a remark for the first unsafe dependence?

llvm/test/Transforms/LoopVectorize/memory-dep-remarks.ll
2

For remarks, it might be good to check the full yaml generated, as in llvm/test/Transforms/LoopVectorize/X86/vectorization-remarks-missed.ll

17

nit: check here not needed?

20

nit: easier to read if this is the last block

122

Is this needed? Same for other tests.

127

is this needed?

265

can the debug lock be trimmed down a bit?

malharJ updated this revision to Diff 396853.Jan 1 2022, 4:54 AM
malharJ marked 8 inline comments as done.
  • minor formatting updates
  • moved remark emission code for unsafe mem dependences into a function
  • got rid of some more debuginfo in the LIT test.
llvm/lib/Analysis/LoopAccessAnalysis.cpp
2148

you are correct, the comment is no longer correct. I've updated it now in the new function.

llvm/test/Transforms/LoopVectorize/memory-dep-remarks.ll
122

Can you please explain why this is not needed ?

127

thanks. removed it now. changed the type of %n from i32 to i64.

265

Done.

  • Changed functions to use same parameters (and same ordering). This helped reduce !DISubroutineType metadata.
  • Also reduced some !DILocation (that were not required) from test_forwardButPreventsForwarding_dep()
malharJ updated this revision to Diff 397552.Jan 5 2022, 6:09 AM
malharJ marked an inline comment as done.
  • Added YAML to the LIT test

Hi @malharJ thanks for splitting up and simplifying this patch, this is an improvement. I left a few more comments, and also found that the remark itself needs a bit of work to separate it from the original message. Maybe you can also update the tests to check the full context, to ensure this no longer happens?

llvm/lib/Analysis/LoopAccessAnalysis.cpp
2141

Can this be an assert?

2148

Can this be an assert?

2154

nit: please move this code below the switch

2158

This dyn_cast will cause a segfault if the value returned by getPointerOperand is a nullptr. This needs to be dyn_cast_or_null. It would be good to have a test for this case.

Also, there is no test for the MemSetInst either. Can you add one?

llvm/test/Transforms/LoopVectorize/memory-dep-remarks.ll
121

When I try this out with Clang, I see:

Use #pragma loop distribute(enable) to allow loop distribution to attempt to isolate the offending operations into a separate loopBackward loop carried data dependence

The issue here is the loopBackward (no period and space between remarks)

malharJ updated this revision to Diff 403808.Jan 27 2022, 2:38 PM
malharJ marked 3 inline comments as done.
  • Rebased patch.
  • Addressed review comments.
llvm/lib/Analysis/LoopAccessAnalysis.cpp
2141

I guess not ...

Looking at the definition of MemoryDepChecker::getDependences(),
it can return a nullptr if RecordDependences is false.
and that happens when number of dependences exceeds MaxDependences.
Given that max-dependences is a command line option, it could have any value.

2148

I dont think so ...

We will still need the check to see if we can find a dependency type that is unsafe for vectorization.

Just the presence of Deps is not sufficient to say above will be satisfied, because it can contain
Forward or Backwardvectorizable dependencies too (but we dont care about emitting any remark for them as they can be vectorized)

2158

Ok, I've changed it to dyn_cast_or_null (I think you had already mentioned it an earlier review comment but I missed it).
Can we please avoid adding more tests, from what I understand it may not be very crucial to test getPointerOperand()
as we just avoid emitting debug location information if it returns null pointer.

Also, I tried writing a test case for memset case and I found that it doesnt even make sense to have a case here for memset.
Apparently LAA does not consider any instruction that is not a load/store
I see this code in LoopAccessInfo::analyzeLoop()

if (!St) {
  recordAnalysis("CantVectorizeInstruction", St)
      << "instruction cannot be vectorized";
  HasComplexMemInst = true;
  continue;
}

So it looks like a different remark is emitted and an early exit is taken because memset is considered as a "complex" memory instruction.

considering the above, I have removed the memset part from the patch.

llvm/test/Transforms/LoopVectorize/memory-dep-remarks.ll
121

I'm not very clear on the issue here .. they are already printed on separate lines ?

Regardless I've updated the tests to print the entire message.
(The reason I was avoiding it earlier was because it was too long a message, and didn't appeal to me as part of the remark.
However, it was not added by my patch so I will not be changing it.)

malharJ updated this revision to Diff 404004.EditedJan 28 2022, 6:29 AM
  • minor update to commit message to trigger phabricator builds.

Update:
Build has failed but test failures are unrelated to this patch.

sdesmalen accepted this revision.Feb 1 2022, 6:08 AM

LGTM with the nits addressed.

llvm/lib/Analysis/LoopAccessAnalysis.cpp
2141

nit: can be removed if you address my comment below.

2143–2151

nit:

if (!Deps || 
    llvm::find_if(Deps, [](const MemoryDepChecker::Dependence &D) { .. }) == Deps->end())
  return;
2185–2192

nit: this is more concisely written as:

if (Instruction *I = Dep.getSource(*this)) {
  DebugLoc SourceLoc = I->getDebugLoc();
  if (auto *DD = dyn_cast_or_null<Instruction>(getPointerOperand(I)))
    SourceLoc = DD->getDebugLoc();
  R << " Memory location is the same as accessed at "
    << ore::NV("Location", SourceLoc);
}
This revision is now accepted and ready to land.Feb 1 2022, 6:08 AM
This revision was landed with ongoing or failed builds.Feb 2 2022, 4:08 AM
This revision was automatically updated to reflect the committed changes.