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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

We should think a bit more about when to use StartBlock and when to use EndBlock. Test cases to find the right choices could be helpful.



================
Comment at: lib/CodeGen/BlockGenerators.cpp:1272
   BasicBlock *BBIDom = DT.getNode(BB)->getIDom()->getBlock();
-  BasicBlock *BBCopyIDom = BlockMap.lookup(BBIDom);
+  BasicBlock *BBCopyIDom = StartBlockMap.lookup(BBIDom);
 
----------------
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?


================
Comment at: lib/CodeGen/BlockGenerators.cpp:1354
+    if (!R->contains(*PI)) {
+      StartBlockMap[*PI] = EntryBBCopy;
+      EndBlockMap[*PI] = EntryBBCopy;
----------------
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.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:1439
+    RegionMap.insert(StartBlockMap.begin(), StartBlockMap.end());
+    ValueMapT &RegionMap2 = RegionMaps[BBCopyEnd];
+    RegionMap2.insert(StartBlockMap.begin(), StartBlockMap.end());
----------------
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.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:1467-1469
+        LoopPHI->addIncoming(LoopPHIInc, StartBlockMap[PredBB]);
       else
+        LoopPHI->addIncoming(NullVal, StartBlockMap[PredBB]);
----------------
Should these be `EndBlockMap`? Since we look for the predecessor of an original block, which is the end of the copy. Test case?


================
Comment at: test/Isl/CodeGen/partial_write_in_region.ll:13
+; CHECK-NEXT:   %scevgep3 = getelementptr float, float* %B, i64 %polly.indvar
+; CHECK-NEXT:   %tmp7_p_scalar_ = load float, float* %scevgep3, align 4, !alias.scope !3, !noalias !4
+; CHECK-NEXT:   %p_tmp8 = fadd float %tmp7_p_scalar_, 1.000000e+00
----------------
Can you remove the references do metadata (`!3` etc) that we are not checking for? I observed metadata numbering sometimes being unstable.


https://reviews.llvm.org/D33767