[PATCH] D26053: [CodeGen] Break critical edge from RTC to original loop.

[Tobias Grosser via llvm-commits]

grosser accepted this revision.
grosser added a comment.
This revision is now accepted and ready to land.

Hi Eli,

this change looks good to me, besides some minor comments. Can you possibly test this change also with POLLY_ENABLE_GPGPU_CODEGEN=ON.

Best,
Tobias

PS: Can you possibly add the [Polly] tag to future  phabricator reviews. This is mostly to help people on the mailing list, as phabricator does not add the project name in its review messages.



================
Comment at: include/polly/CodeGen/BlockGenerators.h:85
+                 ValueMapT &GlobalMap, IslExprBuilder *ExprBuilder,
+                 BasicBlock *StartBlock);
 
----------------
Please document StartBlock.


================
Comment at: include/polly/CodeGen/IslExprBuilder.h:128
                  llvm::ScalarEvolution &SE, llvm::DominatorTree &DT,
-                 llvm::LoopInfo &LI);
+                 llvm::LoopInfo &LI, llvm::BasicBlock *StartBlock);
 
----------------
Could you document StartBlock?

(I know not all parameters are documented. Besides this obviously not being good, this seems useful especially for the change at discussion)


================
Comment at: include/polly/Support/ScopHelper.h:348
+                           llvm::Instruction *IP, ValueMapT *VMap,
+                           llvm::BasicBlock *RTCBB);
 
----------------
Could you document RTCBB?


================
Comment at: lib/CodeGen/Utils.cpp:218
+  //      ExitBB                   //
+  //      /    \                   //
 
----------------
This looks good to me.


================
Comment at: lib/Support/ScopHelper.cpp:232
       : Expander(SCEVExpander(SE, DL, Name)), SE(SE), Name(Name), R(R),
-        VMap(VMap) {}
+        VMap(VMap), RTCBB(RTCBB) {}
 
----------------
I like that this patch makes the insert position of new instructions explicit, as the old code was clearly very fragile.

This change exposes really an unnecessary strong connection between the insert location and the run-time block where we currently insert code to. I wonder if we not even want to just name this InsertBlock instead of RTCBB. 


Repository:
  rL LLVM

https://reviews.llvm.org/D26053