[PATCH] D12766: [Polly] Runtime check elimination

[Johannes Doerfert via llvm-commits]

jdoerfert marked 9 inline comments as done.
jdoerfert added a comment.

@grosser @Meinersbur Thanks for the comments! I'll push an updated value directly into the repo.


================
Comment at: lib/Analysis/ScopDetection.cpp:300
@@ +299,3 @@
+bool ScopDetection::isValidExceptionBlock(BasicBlock &BB) {
+
+  for (Instruction &Inst : BB)
----------------
Meinersbur wrote:
> empty line?
Yes.

================
Comment at: lib/Analysis/ScopDetection.cpp:305
@@ +304,3 @@
+        if (F->getName().equals("__ubsan_handle_out_of_bounds"))
+          return true;
+
----------------
Meinersbur wrote:
> Isn't it possible that __ubsan_handle_out_of_bounds is merged with other code on the same condition?
That does not matter for the whole scheme. We will never model or execute the block anyway.

================
Comment at: lib/Analysis/ScopDetection.cpp:323
@@ +322,3 @@
+  return true;
+}
+
----------------
Meinersbur wrote:
> Could you elaborate why all these conditions are necessary?
Simplified and listed conditions in header description.

================
Comment at: lib/Analysis/ScopDetection.cpp:401
@@ -370,3 +400,3 @@
   if (CI.doesNotReturn())
-    return false;
+    return isValidExceptionBlock(*CI.getParent());
 
----------------
grosser wrote:
> Meinersbur wrote:
> > isValidExceptionBlock also requires that the block has an unreachable terminator, which probably doesn't matter here.
> > 
> > Also, what about such (stupid) code?
> > 
> >    abort()
> >    __ubsan_handle_out_of_bounds()
> >    br somewhere_else
> I would not check the content of an exception block at all. It will anyhow never be executed.
Simplified this wrt to the comments from Tobias.


http://reviews.llvm.org/D12766