[PATCH] D13487: [Polly] Load/Store scalar accesses before/after the statement itself

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Michael,

it seems after r250411 the parts of the patch that caused concerns are not needed any more. Even though they might be replaced soon, I think it is worth committing a minimal patch. I am very bad in keep track of things that should be removed, so while at this lets try to avoid adding unnecessary complicated code (including the PHI node stuff that does not seem to be well understood). Maybe you could rebase the patch once again?

Best,
Tobias


================
Comment at: lib/CodeGen/BlockGenerators.cpp:1170
@@ -1127,1 +1169,3 @@
+               !DT.dominates(cast<Instruction>(Val)->getParent(), StmtExitBB))
+      continue;
 
----------------
Meinersbur wrote:
> Your proposed code fails Isl/CodeGen/phi_in_exit_early_lnt_failure_2.ll  with assertion:
> ```
> Assertion `!verifyGeneratedFunction(S, *EnteringBB->getParent()) && "Verification of generated function failed"' failed.
> ```
I just retested after 250411 and I do not see any failure.

================
Comment at: test/Isl/CodeGen/non-affine-phi-node-expansion-3.ll:20-21
@@ -19,4 +19,4 @@
 ; CHECK-NEXT: %p_val2 = fadd float 1.000000e+00, 2.000000e+00
-; CHECK-NEXT: store float %p_val0, float* %merge.phiops
+; CHECK: store float %p_val0, float* %merge.phiops
 
 branch1:
----------------
Meinersbur wrote:
> ```
>   %val1.s2a.reload = load float, float* %val1.s2a
>   %val2.s2a.reload = load float, float* %val2.s2a
> ```
> appeared because I modified the check in generateScalarLoads that originally was ```isa<PHINode>(Inst)```. The new condition also let loads through that are also not supposed to be there in the first place. I reinstated the original condition although I am not convinced it is correct There could be a legitimate PHI in the subregion's entry block.
> 
> Just to remind, this is going to be cleaned up in D13762 anyway.
After 250411 I do not see such loads being generated even with the just this simple implementation:

```
void RegionGenerator::generateScalarLoads(ScopStmt &Stmt, ValueMapT &BBMap) {
  return BlockGenerator::generateScalarLoads(Stmt, BBMap);
}
```


http://reviews.llvm.org/D13487