[PATCH] D12766: [Polly] Runtime check elimination

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

I am missing a definition (in the source) what a "exception-like block" is and why it can be ignored.

Also, why is UBSan special that it cannot be handled like other exception-like blocks?

Can you give an motivating example?


================
Comment at: include/polly/ScopDetection.h:349
@@ +348,3 @@
+  /// @return True if the block is a valid exception block, false otherwise.
+  static bool isValidExceptionBlock(BasicBlock &BB);
+
----------------
Wouldn't it be better to define static methods as normal functions unless it needs access private static members?

================
Comment at: lib/Analysis/ScopDetection.cpp:299
@@ -298,1 +298,3 @@
 
+bool ScopDetection::isValidExceptionBlock(BasicBlock &BB) {
+
----------------
Judging from when the code calling this, this has nothing to do with exceptions (i.e. "invoke" statement), but just error condition checking.

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

================
Comment at: lib/Analysis/ScopDetection.cpp:305
@@ +304,3 @@
+        if (F->getName().equals("__ubsan_handle_out_of_bounds"))
+          return true;
+
----------------
Isn't it possible that __ubsan_handle_out_of_bounds is merged with other code on the same condition?

================
Comment at: lib/Analysis/ScopDetection.cpp:323
@@ +322,3 @@
+  return true;
+}
+
----------------
Could you elaborate why all these conditions are necessary?

================
Comment at: lib/Analysis/ScopDetection.cpp:401
@@ -370,3 +400,3 @@
   if (CI.doesNotReturn())
-    return false;
+    return isValidExceptionBlock(*CI.getParent());
 
----------------
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

================
Comment at: lib/Analysis/ScopDetection.cpp:710
@@ -709,3 +739,3 @@
   L->getExitingBlocks(ExitingBlocks);
-  for (BasicBlock *ExitingBB : ExitingBlocks) {
     if (!isValidCFG(*ExitingBB, Context))
----------------
unrelated change


http://reviews.llvm.org/D12766