[PATCH] D37296: [ScopInfo] Use statement lists for entry blocks of region statements

[Michael Kruse via Phabricator via llvm-commits]

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

I don't see any correctness issues: LGTM.



================
Comment at: include/polly/ScopInfo.h:1209
+  ScopStmt(Scop &parent, Region &R, Loop *SurroundingLoop,
+           std::vector<Instruction *> EntryBlockInstructions);
 
----------------
Instead of passing `std::vector` by value, wouldn't it be better to pass an `ArrayRef`?


================
Comment at: lib/Analysis/ScopBuilder.cpp:667-670
+    for (Instruction &Inst : *SR.getEntry())
+      if (!isa<TerminatorInst>(&Inst) && !isIgnoredIntrinsic(&Inst) &&
+          !canSynthesize(&Inst, *scop, &SE, SurroundingLoop))
+        Instructions.push_back(&Inst);
----------------
Can we refactor this shared code (with block stmts) into e.g. common function?


================
Comment at: lib/Analysis/ScopInfo.cpp:1684
+      Instructions(EntryBlockInstructions)
+
+{
----------------
Empty line?


================
Comment at: lib/Analysis/ScopInfo.cpp:3596-3597
+  if (Stmt.isRegionStmt()) {
+    for (Instruction *Inst : Stmt.getInstructions())
+      InstStmtMap.erase(Inst);
     for (BasicBlock *BB : Stmt.getRegion()->blocks()) {
----------------
This code is also shared with the block stmt case.


================
Comment at: lib/Analysis/ScopInfo.cpp:3600-3601
       StmtMap.erase(BB);
+      if (BB == Stmt.getEntryBlock())
+        continue;
       for (Instruction &Inst : *BB)
----------------
Should this be
```
     if (BB != Stmt.getEntryBlock())
        continue;
```
?


================
Comment at: lib/CodeGen/BlockGenerators.cpp:458
+  // instruction list is not yet supported.
+  if (Stmt.isBlockStmt() || (Stmt.isRegionStmt() && Stmt.getEntryBlock() == BB))
     for (Instruction *Inst : Stmt.getInstructions())
----------------
Simpler: `Stmt.getEntryBlock() == BB` which should work for the block stmt and region stmt case. 

However, your version makes it more explicit what was intended.


https://reviews.llvm.org/D37296