This is an archive of the discontinued LLVM Phabricator instance.

[Polly] [PPCGCodeGeneration] Deal with loops outside the Scop correctly in PPCGCodeGeneration.
ClosedPublic

Authored by bollu on Aug 3 2017, 3:02 PM.

Details

Summary

A Scop with a loop outside it is not handled currently by
PPCGCodeGeneration. The test case is such that the Scop has only one inner loop
that is detected. This currently breaks codegen.

The fix is to reuse the existing mechanism in IslNodeBuilder within
`GPUNodeBuilder.

Event Timeline

bollu created this revision.Aug 3 2017, 3:02 PM
bollu added inline comments.Aug 3 2017, 3:11 PM
lib/CodeGen/PPCGCodeGeneration.cpp
1416

@grosser - Can you explain to me why the other condition exists? (L->contains(S.getEntry()))

I removed this extra condition from IslNodeBuilder and all our checks pass. git blame is somewhat unhelpful, since it points to a commit where IslNodeBuilder was extracted out.

If you want to verify,

remove-extra-check.patch
diff --git a/lib/CodeGen/IslNodeBuilder.cpp b/lib/CodeGen/IslNodeBuilder.cpp
index ecc84f40..7474af4b 100644
--- a/lib/CodeGen/IslNodeBuilder.cpp
+++ b/lib/CodeGen/IslNodeBuilder.cpp
@@ -313,7 +313,7 @@ void IslNodeBuilder::getReferencesInSubtree(__isl_keep isl_ast_node *For,
   /// are considered local. This leaves only loops that are before the scop, but
   /// do not contain the scop itself.
   Loops.remove_if([this](const Loop *L) {
-    return S.contains(L) || L->contains(S.getEntry());
+    return S.contains(L); // || L->contains(S.getEntry());
   });

   // Contains Values that may need to be replaced with other values
grosser requested changes to this revision.Aug 3 2017, 11:53 PM

Added a comment. Waiting for reply.

lib/CodeGen/PPCGCodeGeneration.cpp
1416

In fact, this seems to be exactly the loops we want to keep to fix the bug we are seeing. Did you run your test-case with the OpenMP code generation, I wonder if it suffers from the very same bug and your test case also fails. In case it does, I would suggest to add the test case, to drop the second condition, and to update the comment.

This revision now requires changes to proceed.Aug 3 2017, 11:53 PM
bollu updated this revision to Diff 109705.Aug 4 2017, 4:25 AM
bollu edited edge metadata.
  • [Works] [WIP] recreate whatever IslNodeBuilder does in PPCGCodeGeneration
bollu updated this revision to Diff 109714.Aug 4 2017, 5:07 AM
  • [NFC] remove copied code, add a comment explaining some dataflow
bollu updated this revision to Diff 109718.Aug 4 2017, 5:24 AM
  • [Polly] [WIP] [PPCGCodeGeneration] Deal with loops outside the Scop correctly in PPCGCodeGeneration.
  • [WIP] try to reuse IslNodeBuilder machinery to reduce differences
  • [Works] [WIP] recreate whatever IslNodeBuilder does in PPCGCodeGeneration
  • [NFC] removing weird comments from code
  • [NFC] remove copied code, add a comment explaining some dataflow
  • [NFC] remove comment
bollu added inline comments.Aug 4 2017, 5:27 AM
lib/CodeGen/PPCGCodeGeneration.cpp
1416

Writing this up for documentation: OpenMP succeeds because it populates the IslNodeBuilder::OutsideLoopIterations list at IslNodeBuilder::materialiseParameters.

It adds everything in OutsideLoopIterations at IslNodeBuilder::getReferencesInSubtree.

This is somewhat action-at-a-distance, but then, that's true of everything with mutation so I'm not sure how to structure it better.

bollu updated this revision to Diff 109720.Aug 4 2017, 5:29 AM
  • [NFC] whitespace cleanup
bollu retitled this revision from [Polly] [WIP] [PPCGCodeGeneration] Deal with loops outside the Scop correctly in PPCGCodeGeneration. to [Polly] [PPCGCodeGeneration] Deal with loops outside the Scop correctly in PPCGCodeGeneration..Aug 4 2017, 5:30 AM

@grosser, @singam-sanjay - Review, please.

grosser accepted this revision.Aug 4 2017, 5:37 AM

LGTM

lib/CodeGen/PPCGCodeGeneration.cpp
1416

Could you add a comment why which loops we exclude and why?

This revision is now accepted and ready to land.Aug 4 2017, 5:37 AM
philip.pfaffe added inline comments.Aug 4 2017, 5:44 AM
lib/CodeGen/PPCGCodeGeneration.cpp
419

You should update the doxygen accordingly

This revision was automatically updated to reflect the committed changes.