[PATCH] D33767: [Polly] [BlockGen] Support partial writes in regions

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Michael,

thank you for the comments. I believe this is now (close to) commit ready, but I would appreciate a final OK. Hope with this change, we can turn the buildbots green!



================
Comment at: lib/CodeGen/BlockGenerators.cpp:1272
   BasicBlock *BBIDom = DT.getNode(BB)->getIDom()->getBlock();
-  BasicBlock *BBCopyIDom = BlockMap.lookup(BBIDom);
+  BasicBlock *BBCopyIDom = StartBlockMap.lookup(BBIDom);
 
----------------
Meinersbur wrote:
> Should this be `EndBlockMap`? Because it is the end of the previous BB that dominates BBCopy.
> 
> I changed it in the code and nothing changes. Is there a test case such that we could test this?
I expanded the test case to cover this case. In fact, we had to lookup the dominator in EndBlockMap and had to return the lookup from StartBlockMap to make sure the correct definitions are provided.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:1354
+    if (!R->contains(*PI)) {
+      StartBlockMap[*PI] = EntryBBCopy;
+      EndBlockMap[*PI] = EntryBBCopy;
----------------
Meinersbur wrote:
> I'd expect only the end block ever by used (to repair dominance or PHI incoming blocks), but I also think it is safer to just apply it.
OK, I leave it then.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:1439
+    RegionMap.insert(StartBlockMap.begin(), StartBlockMap.end());
+    ValueMapT &RegionMap2 = RegionMaps[BBCopyEnd];
+    RegionMap2.insert(StartBlockMap.begin(), StartBlockMap.end());
----------------
Meinersbur wrote:
> Call it `RegionMapEnd`?
> 
> Do we even need it? I removed it and tests still pass. Having only ValueMap per BB is easier to maintain (otherwise why shouldn't the *.partial blocks have one as well.). The call to `copyInstScalar` below will only update one of them: `RegionMap`, i.e. the EndBlock's value map will necessarily out of date. We could also keep the EndBlock's value map, because only in that one all values would be available.
No, we do not need it. I dropped it. The only place where we would have needed is was in add addOperandToPHI, which I now changed to lookup the necessary values in RegionMaps[BBCopyStart].


================
Comment at: lib/CodeGen/BlockGenerators.cpp:1467-1469
+        LoopPHI->addIncoming(LoopPHIInc, StartBlockMap[PredBB]);
       else
+        LoopPHI->addIncoming(NullVal, StartBlockMap[PredBB]);
----------------
Meinersbur wrote:
> Should these be `EndBlockMap`? Since we look for the predecessor of an original block, which is the end of the copy. Test case?
It should. Due to https://bugs.llvm.org//show_bug.cgi?id=33265, I had some issues to create a working test case, but now added one with 
test/Isl/CodeGen/partial_write_in_region_with_loop.ll.


https://reviews.llvm.org/D33767