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

[Tobias Grosser via Phabricator via llvm-commits]

grosser marked an inline comment as done.
grosser added inline comments.


================
Comment at: include/polly/ScopInfo.h:1209
+  ScopStmt(Scop &parent, Region &R, Loop *SurroundingLoop,
+           std::vector<Instruction *> EntryBlockInstructions);
 
----------------
Meinersbur wrote:
> Instead of passing `std::vector` by value, wouldn't it be better to pass an `ArrayRef`?
Possibly, but I wanted to mirror the existing code. I will commit this as a follow up cleanup.


================
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);
----------------
Meinersbur wrote:
> Can we refactor this shared code (with block stmts) into e.g. common function?
I extracted out a method shouldModelInst. I could not include the loop, as I did not yet want to allow statement
splitting in region statements.


================
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()) {
----------------
Meinersbur wrote:
> This code is also shared with the block stmt case.
Pulled out.


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

```
    // Skip entry basic block, as its instructions are already deleted as      
    // part of the statement's instruction list.
```


================
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())
----------------
Meinersbur wrote:
> 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.
OK, then I will leave my code.


https://reviews.llvm.org/D37296