[PATCH] D12053: [Polly] Workaround for SDiv/SRem referenced from SCEVExpander

[Johannes Doerfert via llvm-commits]

jdoerfert added a comment.

Some comments and a proposal.


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:58
@@ -53,1 +57,3 @@
+}
+
 __isl_give isl_ast_expr *
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > Why do we need this?
> LLVM defines SDivOperator, but not SRemOperator
Why don't you cast to SRem and SDiv instructions instead of operators?

================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:724
@@ +723,3 @@
+      if (isa<SDivOperator>(Inst) || isa<SRemOperator>(Inst))
+        Worklist.push_back(&cast<Instruction>(Inst));
+    }
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > What type does Inst have if not Instruction &?
> The worklist push below is of type llvm::User*, but it might indeed be unnecessary here (and below since the cast has already been done)
I do not see anything of type User between 720 and 727 but as long as we agree that the cast is not necessary I don't care.

================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:737
@@ +736,3 @@
+      continue;
+
+    bool DepWithinRegion = false;
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > Can we just copy it without all the checks if we can move it?
> The move in the comment is outdated. I tried to move it in an previous implementation and found that moving doesn't come without problems.
> 
> There is still nothing to do if SCEVValidator rejected it. 
Wait, moving doesn't come with problems or does? If the latter, which problems?

My proposal is __not__ to move instructions at all but copy the SDiv and SRem instructions including the operands trees to the new location. Can you comment on that?


http://reviews.llvm.org/D12053