Page MenuHomePhabricator

[LoopVectorize] Emit runtime checks correctly for nested loops
Needs RevisionPublic

Authored by kpdev42 on Aug 23 2022, 10:52 AM.

Details

Summary

Fixes https://github.com/llvm/llvm-project/issues/57315

~~~

OS Laboratory. Huawei RRI. Saint-Petersburg

Diff Detail

Event Timeline

kpdev42 created this revision.Aug 23 2022, 10:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 10:52 AM
kpdev42 requested review of this revision.Aug 23 2022, 10:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 10:52 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
kpdev42 updated this revision to Diff 454906.Aug 23 2022, 11:18 AM

Remove unrelated changes

fhahn added a comment.Aug 24 2022, 5:47 AM

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

Allen added a subscriber: Allen.Aug 24 2022, 7:48 AM
kpdev42 updated this revision to Diff 455303.Aug 24 2022, 11:29 AM

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.

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.

Thank you fer the review.
Lets keep buildbot green. Please land both fix and test case on your own

kpdev42 edited the summary of this revision. (Show Details)Aug 25 2022, 12:37 AM

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.

Thank you fer the review.
Lets keep buildbot green. Please land both fix and test case on your own

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.

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.

Thank you fer the review.
Lets keep buildbot green. Please land both fix and test case on your own

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.

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?

fhahn requested changes to this revision.Sep 26 2022, 6:46 AM

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.

Thank you fer the review.
Lets keep buildbot green. Please land both fix and test case on your own

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.

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?

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.

This revision now requires changes to proceed.Sep 26 2022, 6:46 AM