[PATCH] D36920: [GPGPU] Collect parameter dimension used in MemoryAccesses

[Siddharth via Phabricator via llvm-commits]

bollu requested changes to this revision.
bollu added a comment.
This revision now requires changes to proceed.

I would like to discuss the `isl::space ParamSpace` issue before LGTM'ing this patch.



================
Comment at: include/polly/CodeGen/IslNodeBuilder.h:47
   BlockGenerator &BlockGen;
+  isl::space ParamSpace;
 };
----------------
Please see the comment about using `isl::space *` here to broadcast the fact that this is an optional member of the struct. 


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:231
 
   for (auto &Access : *Stmt) {
+    isl::space ParamSpace = Access->getLatestAccessRelation().get_space();
----------------
Nit: Change the `auto` to `MemoryAccess`? It would not be obvious to someone who does not work on Polly that a statement iterator gives `MemoryAccess`.


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:303
   struct SubtreeReferences References = {
-      LI, SE, S, ValueMap, Values, SCEVs, getBlockGenerator()};
+      LI, SE, S, ValueMap, Values, SCEVs, getBlockGenerator(), nullptr};
 
----------------
We abuse the fact that `nullptr` operations just propagate `nullptr` in ISL. I'm trying to think of neater ways to express this. I _really_ abusing `nullptr` this way since it is quite literally dependent on the ISL implementation.

For now, we can make it a `isl::space *` to express the fact that it a choice. Ideally, we could somehow split the different "visitors" so that `ISLNodeBuilder` would not use the `parameter` visitor but `GPUNodeBuilder` could.


================
Comment at: test/GPGPU/memory-only-referenced-from-access.ll:49
+
+!0 = !{!1}
+!1 = distinct !{!1, !2, !"__radiation_rg_MOD_coe_so: %pa1f"}
----------------
Please remove the metadata which is not required.


https://reviews.llvm.org/D36920