[PATCH] D36425: [ZoneAlgo] Allow two writes that write identical values into same array slot

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:199-200
 
-  Info.TaggedMustKills = isl::union_map::empty(isl::space(ParamSpace));
-  Info.MustKills = isl::union_map::empty(isl::space(ParamSpace));
+  Info.TaggedMustKills = isl::union_map::empty(ParamSpace);
+  Info.MustKills = isl::union_map::empty(ParamSpace);
 
----------------
[Nit] unrelated?


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:228
     // 2a. [param] -> { Stmt[] -> scalar_to_kill[] }
-    isl::map StmtToScalar = isl::map::universe(isl::space(ParamSpace));
+    isl::map StmtToScalar = isl::map::universe(ParamSpace);
     StmtToScalar = StmtToScalar.set_tuple_id(isl::dim::in, isl::id(KillStmtId));
----------------
[Nit] unrelated?


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:237
     // 2b. [param] -> { phantom_ref[] -> scalar_to_kill[] }
-    isl::map PhantomRefToScalar = isl::map::universe(isl::space(ParamSpace));
+    isl::map PhantomRefToScalar = isl::map::universe(ParamSpace);
     PhantomRefToScalar =
----------------
[Nit] unrelated?


================
Comment at: lib/Transform/ZoneAlgo.cpp:291
+
+  for (auto *MA : *Stmt) {
+    if (!MA->isLatestArrayKind() || !MA->isMustWrite() ||
----------------
[Suggestion] This will reject as soon as there is a third store that writes a different value somewhere unrelated. Maybe add a TODO comment and/or add a `FAIL: *` test case to improve this some day?


================
Comment at: lib/Transform/ZoneAlgo.cpp:292-293
+  for (auto *MA : *Stmt) {
+    if (!MA->isLatestArrayKind() || !MA->isMustWrite() ||
+        !MA->isOriginalArrayKind())
+      continue;
----------------
[Suggestion] Does either `isLatestArrayKind()` or `MA->isOriginalArrayKind()` suffice?
(I think we only implemented changing scalar to array access, not the other way around; scalar accesses are assumed to either at the beginning or end of the statement, never associated with a LoadInst/StoreInst)


================
Comment at: test/ForwardOpTree/forward_load_double_write.ll:3
+;
+; Rematerialize a load.
+;
----------------
[?] Update the comment what's tested here.


https://reviews.llvm.org/D36425