[PATCH] D12766: [Polly] Runtime check elimination

[Tobias Grosser via llvm-commits]

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

Hi Johannes,

this patch looks nice and clean! Very cool. Just some smaller comments inline. Besides those, I think it would be good to add an ASAN test case and a multi-dimensional array test case.

Best,
Tobias


================
Comment at: lib/Analysis/ScopDetection.cpp:320
@@ +319,3 @@
+    return CI && CI->doesNotReturn();
+  }
+
----------------
Also, It does not seem to matter what the instructions do. As long as the block is unreachable, I think we can decide to ignore it.

================
Comment at: lib/Analysis/ScopDetection.cpp:322
@@ +321,3 @@
+
+  return true;
+}
----------------
I generally prefer to have 'return false' as the default at the end and then only return true for the cases we indeed want to handle.

Also, it is unclear to me why the last three checks are needed. You seem to very detailed check that this is really an exception block. I would just handle anything that ends in an unreachable as such.

================
Comment at: lib/Analysis/ScopDetection.cpp:401
@@ -370,3 +400,3 @@
   if (CI.doesNotReturn())
-    return false;
+    return isValidExceptionBlock(*CI.getParent());
 
----------------
I would not check the content of an exception block at all. It will anyhow never be executed.

================
Comment at: lib/Analysis/ScopDetection.cpp:929
@@ -899,3 +928,3 @@
   for (BasicBlock *BB : CurRegion.blocks())
     if (!isValidCFG(*BB, Context) && !KeepGoing)
       return false;
----------------
I would skip exception blocks already here. There content does not really matter, no?

================
Comment at: test/ScopInfo/BoundChecks/single-loop.ll:32
@@ -22,1 +31,3 @@
+; AST: else
+; AST:     {  /* original code */ }
 
----------------
Nice!


http://reviews.llvm.org/D12766