[PATCH] D5227: [Polly] Support ModRef function behaviour

[Johannes Doerfert via llvm-commits]

jdoerfert added a comment.

I guess this patch is close to landing. I wait for Michaels reply though.

I also discussed some extended use today with Philip Pfaffe from KIT, but I will start an email thread for that one.


================
Comment at: lib/Analysis/ScopDetection.cpp:476
@@ +475,3 @@
+  case llvm::FMRB_UnknownModRefBehavior:
+    return false;
+  case llvm::FMRB_DoesNotAccessMemory:
----------------
Meinersbur wrote:
> The same approach as for FMRB_OnlyReadsMemory could be taken here, but with "GlobalWrites", also known as [[ https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Clobbers | Clobbers ]].
Yes, that is a good remark. I would like to push that only when we have more infrastructure/heuristics in-place that can determine if we should exit early. As a start we could "not count" all loops that contain a UnknownModRefBehaviour call in their body. This way the current heuristic would kick in if the loop is trivially sequential. Do you agree we can move forward on this in a follow up patch?

================
Comment at: lib/Analysis/ScopInfo.cpp:3979
@@ +3978,3 @@
+      addArrayAccess(Inst, AccType, ArgBasePtr->getValue(),
+                     ArgBasePtr->getType(), false, {AF}, {}, &CI);
+    }
----------------
Meinersbur wrote:
> What happens if there are other multi-dimensional accesses to the same array exist?
This is also a really good point! Thanks! I will add a test case and add code to the ScopDetection that will prevent us to delinearize a base pointer that is used in such a call (The same problem is actually present for MemIntrinsics, thus I will push the check and another test prior to this one). Later we can lift that restriction and make the access we create here multidimensional. Again I would like to do that in a follow-up patch if you agree.

================
Comment at: lib/Analysis/ScopInfo.cpp:4308
@@ +4307,3 @@
+  for (auto *GlobalRead : GlobalReads)
+    for (auto *BP : ArrayBasePointers)
+      addArrayAccess(MemAccInst(GlobalRead), MemoryAccess::READ, BP,
----------------
Meinersbur wrote:
> Can't we iterate over all ScopArrayInfo's of type MK_Array instead of introducing a new list ArrayBasePointers?
I tried and I failed to do that. The reason is the late creation of the ScopArrayInfo objects. At that point in time we do no have access to the ScopInfo, the "addArrayAccess" method and a couple of other things. I do agree it would be nicer, though I also see a benefit in keeping the creation of the SCoP in the ScopInfo pass. Is it OK to keep it like this for now? [Assuming you don't have a good way to call/emulate addArrayAccess after the ScopArrayInfo objects have been created. If this assumption is wrong I can change the patch immediately.]]

================
Comment at: test/ScopInfo/mod_ref_access_pointee_arguments.ll:7-17
@@ +6,13 @@
+;
+; CHECK: Stmt_for_body
+; CHECK:   Domain :=
+; CHECK:       { Stmt_for_body[i0] : 0 <= i0 <= 1023 };
+; CHECK:   Schedule :=
+; CHECK:       { Stmt_for_body[i0] -> [i0] };
+; CHECK:   MayWriteAccess := [Reduction Type: NONE]
+; CHECK:       { Stmt_for_body[i0] -> MemRef_A[o0] };
+; CHECK:   ReadAccess := [Reduction Type: NONE]
+; CHECK:       { Stmt_for_body[i0] -> MemRef_B[i0] };
+; CHECK:   MustWriteAccess :=  [Reduction Type: NONE]
+; CHECK:       { Stmt_for_body[i0] -> MemRef_A[i0] };
+;
----------------
Meinersbur wrote:
> I think we want to go to the CHECK-NEXT style.
I ported the test case. My bad. I will change them.


http://reviews.llvm.org/D5227