Fixes https://github.com/llvm/llvm-project/issues/57315
~~~
OS Laboratory. Huawei RRI. Saint-Petersburg
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Thanks for the patch! I don't think this is the ideal solution, it looks like the source of the issue is a missing check that the AddRecs for step expressions are both in the inner loop. I'll include a diff with the fix below which I am planning to submit.
As you already shared a test case, it would be great if you could clean it up a bit and land just the patch to start with. I left some comments inline. What do you think?
index 5a01a8e4b055..220e48643245 100644 --- a/llvm/include/llvm/Analysis/LoopAccessAnalysis.h +++ b/llvm/include/llvm/Analysis/LoopAccessAnalysis.h @@ -253,6 +253,10 @@ public: return {}; } + const Loop *getInnermostLoop() const { + return InnermostLoop; + } + private: /// A wrapper around ScalarEvolution, used to add runtime SCEV checks, and /// applies dynamic knowledge to simplify SCEV expressions and convert them diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp index a9a4a820db50..4febf0bfd62c 100644 --- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp +++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp @@ -281,7 +281,7 @@ void RuntimePointerChecking::tryToCreateDiffCheck( auto *SrcAR = dyn_cast<SCEVAddRecExpr>(Src->Expr); auto *SinkAR = dyn_cast<SCEVAddRecExpr>(Sink->Expr); - if (!SrcAR || !SinkAR) { + if (!SrcAR || !SinkAR || SrcAR->getLoop() != DC.getInnermostLoop() || SinkAR->getLoop() != DC.getInnermostLoop()) { CanUseDiffCheck = false; return; }
| llvm/test/Transforms/LoopVectorize/nested-loop.ll | ||
|---|---|---|
| 9 | I don't think this test needs the X86 cost model. target triple could probably be dropped and replaced by passing -force-vector-width=4 -force-vector-interleave=1 to opt. If it requires the target triple, it needs to be moved to the X86 sub-directory. | |
| 17 | This check shouldn't be really needed. | |
| 20 | could pass %Len as i64 | |
| 23 | block names could be simplified. e.g. %for.cond1.preheader.us -> %outer.header, %for.cond1.for.end_crit_edge.us: -> %outer.latch, %for.body3.us -> %inner for.end12 -> %exit. | |
| 31 | same could be simplified by dropping indvars. prefix. | |
| 35 | !tbaa shouldn't be needed. | |
| 42 | This could be simplified, if a use of %sub.us.lcssa is needed then a function that only takes an i32 should be sufficient. | |
| 57 | attributes shouldn't be needed | |
| 60 | none of the metadata should be needed | |
Comment Actions
Thanks for the update. As I mentioned in the previous message, I think it would be good to just land the test in this patch and I'll submit the code change separately. The test looks good to me.
Comment Actions
Thank you fer the review.
Lets keep buildbot green. Please land both fix and test case on your own
Comment Actions
Thanks, the idea would be to commit the test first with the checks so it passes without the fix. The fix then only shows the improvements on the test case, so the bots would stay green at all time.
Comment Actions
I am afraid that this test does not pass without fix, so far would you like to (a) modify the test, so that it would pass (diff check instead of overlap check) or (b) XFAIL the test or (c) maybe something else?
Comment Actions
Oh right, common practice is to generate the (incorrect) check lines for the test so they pass on current main, together with a FIXME comment explaining what the current issue is. I did something like that in 6e56779e6bc168a3acd14f9bf2c4fd3fd9d86bd1.
Anyways, it should be fixed on current main by now.
I don't think this test needs the X86 cost model. target triple could probably be dropped and replaced by passing -force-vector-width=4 -force-vector-interleave=1 to opt. If it requires the target triple, it needs to be moved to the X86 sub-directory.