[Polly] [NFC] [ScopDetection] Make `polly-only-func` perform regex scop name match.

[bollu]

Summary

• We were using substring matching for polly-only-func.
• Generalise this, so allow regexes as a parameter to polly-only-func.

[grosser]

Cool!

[Meinersbur]

lib/Analysis/ScopDetection.cpp

285 ↗

(On Diff #107676)

[Serious] Don't use llvm_unreachable to reliably abort. In release-builds something unpredictable will happen, e.g. remove the R.isValid test altogether because there is just one outcome with defined behavior.

[grosser]

Thanks Michael!

lib/Analysis/ScopDetection.cpp

285 ↗

(On Diff #107676)

Right. I missed this one!

[bollu]

lib/Analysis/ScopDetection.cpp

285 ↗

(On Diff #107676)

What does:

remove the R.isValid test altogether because there is just one outcome with defined behavior.

mean?

If I cannot guarantee llvm_unreachable will abort, would returning false be acceptable? I really want to keep the error logging because I can forsee myself spending a good couple of minutes trying to debug this at some point in the future :).

[bollu]

[NFC wrt patch] return false indicating failure of regex match if regex did not compile

[bollu]

• [NFC] diff against master.

[bollu]

lib/Analysis/ScopDetection.cpp

285 ↗

(On Diff #107676)

@Meinersbur does this look okay?

[singam-sanjay]

lib/Analysis/ScopDetection.cpp

285 ↗

(On Diff #107676)

@bollu You still have llvm_unreachable before return false;. I'm not sure if that was intentional.

[bollu]

lib/Analysis/ScopDetection.cpp

285 ↗

(On Diff #107676)

Yes, because I don't want it to "reliably abort", I want it to abort in debug builds.

[grosser]

My feeling is that the user-observable behavior of debug and non-debug builds should be indentical. If this is not the case, I would see this as a bug.

[bollu]

@Meinersbur: I would like an LGTM on your part to be comfortable with upstreaming this.

[bollu]

@grosser - Then is every use of llvm_unreachable a bug?

[grosser]

@bollu: Every use where someone can create an input that triggers it.

[Meinersbur]

lib/Analysis/ScopDetection.cpp

285 ↗

(On Diff #107676)

A llvm_unreachable means that the programmer thinks that that line can under no circumstance can execute. Executing it is undefined behaviour. That the program terminates with a message is a courtesy of the Debug build. In release builds, the compiler can remove the entire branch because it thinks there is no execution with defined behaviour that executes it.

See the __builtin_unreachable section at https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html .

Relevant comments in the llvm sources:

/// LLVM_BUILTIN_UNREACHABLE - On compilers which support it, expands
/// to an expression which states that it is undefined behavior for the
/// compiler to reach this point.  Otherwise is not defined.

/// In NDEBUG builds, becomes an optimizer hint that the current location
/// is not supposed to be reachable.  On compilers that don't support
/// such hints, prints a reduced message instead.
///
/// Use this instead of assert(0).  It conveys intent more clearly and
/// allows compilers to omit some unnecessary code.

So llvm_unreachable is only a substitute for assert(false).

In C programs, use abort() to exit the program under an error condition.

The LLVM command line paser calls report_fatal_error() with malformed user input. I suggest to do the same.

[bollu]

• [NFC wrt patch] change to use report_fatal_error

[bollu]

• [NFC wrt patch] removed return false because we call a noreturn function anyway

[grosser]

lib/Analysis/ScopDetection.cpp

285 ↗

(On Diff #107676)

You can also drop the braces around a single statement body

polly/trunk/lib/Analysis/ScopDetection.cpp

 #include "llvm/Analysis/ScalarEvolution.h"
 #include "llvm/Analysis/ScalarEvolutionExpressions.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Regex.h"
 #include <set>
 #include <stack>
 
 using namespace llvm;
 using namespace polly;
 
 #define DEBUG_TYPE "polly-detect"
 
static cl::opt<bool, true> XPollyProcessUnprofitable(
     "polly-process-unprofitable",
     cl::desc(
         "Process scops that are unlikely to benefit from Polly optimizations."),
     cl::location(PollyProcessUnprofitable), cl::init(false), cl::ZeroOrMore,
     cl::cat(PollyCategory));
 
 static cl::list<std::string> OnlyFunctions(
     "polly-only-func",
-    cl::desc("Only run on functions that contain a certain string. "
-             "Multiple strings can be comma separated. "
-             "Scop detection will run on all functions that contain "
-             "any of the strings provided."),
+    cl::desc("Only run on functions that match a regex. "
+             "Multiple regexes can be comma separated. "
+             "Scop detection will run on all functions that match "
+             "ANY of the regexes provided."),
     cl::ZeroOrMore, cl::CommaSeparated, cl::cat(PollyCategory));
 
 static cl::opt<bool>
     AllowFullFunction("polly-detect-full-functions",
                       cl::desc("Allow the detection of full functions"),
                       cl::init(false), cl::cat(PollyCategory));
 
 static cl::opt<std::string> OnlyRegion(
if (FileName.empty()) {
     return;
   }
 
   DP << FileName << ":" << EntryLine << ": Start of scop\n";
   DP << FileName << ":" << ExitLine << ": End of scop";
 }
 
 static bool IsFnNameListedInOnlyFunctions(StringRef FnName) {
-  for (auto Name : OnlyFunctions)
-    if (FnName.count(Name) > 0)
+  for (auto RegexStr : OnlyFunctions) {
+    Regex R(RegexStr);
+
+    std::string Err;
+    if (!R.isValid(Err))
+      report_fatal_error(
+          "invalid regex given as input to -polly-only-func. " + Err, true);
+
+    if (R.match(FnName))
       return true;
+  }
   return false;
 }
 //===----------------------------------------------------------------------===//
 // ScopDetection.
 
 ScopDetection::ScopDetection(Function &F, const DominatorTree &DT,
                              ScalarEvolution &SE, LoopInfo &LI, RegionInfo &RI,
                              AliasAnalysis &AA, OptimizationRemarkEmitter &ORE)

polly/trunk/test/ScopDetect/only_func_flag_regex.ll

+; RUN: opt %loadPolly -polly-scops  -analyze -polly-only-func=f.*,g.* < %s | FileCheck %s
+;
+; Check that the flag `-polly-only-func` works with regexes.
+;
+; CHECK:      Function: f1
+; CHECK-NEXT:    Region: %for.cond---%for.end
+;
+; CHECK:      Function: f2
+; CHECK-NEXT:    Region: %for.cond---%for.end
+;
+; CHECK:      Function: g1
+; CHECK-NEXT:    Region: %for.cond---%for.end
+;
+; CHECK-NOT:      Function: h
+;
+; void f1(int* sum) {
+;   for (int i = 0; i <= 100; i++)
+;     sum += i * 3;
+; }
+; void f2(int* sum) {
+;   for (int i = 0; i <= 100; i++)
+;     sum += i * 3;
+; }
+; void g1(int* sum) {
+;   for (int i = 0; i <= 100; i++)
+;     sum += i * 3;
+; }
+; void h(int* sum) {
+;   for (int i = 0; i <= 100; i++)
+;     sum += i * 3;
+; }
+target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-n32-S64"
+
+define void @f1(i32* %sum) {
+entry:
+  br label %entry.split1
+
+entry.split1:                                     ; preds = %entry
+  br label %entry.split
+
+entry.split:                                      ; preds = %entry.split1
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.cond, %entry.split
+  %i1.0 = phi i32 [ 0, %entry.split ], [ %inc, %for.cond ]
+  %sum.reload = load i32, i32* %sum
+  %mul = mul nsw i32 %i1.0, 3
+  %add = add nsw i32 %sum.reload, %mul
+  %inc = add nsw i32 %i1.0, 1
+  store i32 %add, i32* %sum
+  %cmp = icmp slt i32 %i1.0, 100
+  br i1 %cmp, label %for.cond, label %for.end
+
+for.end:                                          ; preds = %for.cond
+  ret void
+}
+
+
+define void @f2(i32* %sum) {
+entry:
+  br label %entry.split1
+
+entry.split1:                                     ; preds = %entry
+  br label %entry.split
+
+entry.split:                                      ; preds = %entry.split1
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.cond, %entry.split
+  %i1.0 = phi i32 [ 0, %entry.split ], [ %inc, %for.cond ]
+  %sum.reload = load i32, i32* %sum
+  %mul = mul nsw i32 %i1.0, 3
+  %add = add nsw i32 %sum.reload, %mul
+  %inc = add nsw i32 %i1.0, 1
+  store i32 %add, i32* %sum
+  %cmp = icmp slt i32 %i1.0, 100
+  br i1 %cmp, label %for.cond, label %for.end
+
+for.end:                                          ; preds = %for.cond
+  ret void
+}
+
+define void @g1(i32* %sum) {
+entry:
+  br label %entry.split1
+
+entry.split1:                                     ; preds = %entry
+  br label %entry.split
+
+entry.split:                                      ; preds = %entry.split1
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.cond, %entry.split
+  %i1.0 = phi i32 [ 0, %entry.split ], [ %inc, %for.cond ]
+  %sum.reload = load i32, i32* %sum
+  %mul = mul nsw i32 %i1.0, 3
+  %add = add nsw i32 %sum.reload, %mul
+  %inc = add nsw i32 %i1.0, 1
+  store i32 %add, i32* %sum
+  %cmp = icmp slt i32 %i1.0, 100
+  br i1 %cmp, label %for.cond, label %for.end
+
+for.end:                                          ; preds = %for.cond
+  ret void
+}
+
+define void @h(i32* %sum) {
+entry:
+  br label %entry.split1
+
+entry.split1:                                     ; preds = %entry
+  br label %entry.split
+
+entry.split:                                      ; preds = %entry.split1
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.cond, %entry.split
+  %i1.0 = phi i32 [ 0, %entry.split ], [ %inc, %for.cond ]
+  %sum.reload = load i32, i32* %sum
+  %mul = mul nsw i32 %i1.0, 3
+  %add = add nsw i32 %sum.reload, %mul
+  %inc = add nsw i32 %i1.0, 1
+  store i32 %add, i32* %sum
+  %cmp = icmp slt i32 %i1.0, 100
+  br i1 %cmp, label %for.cond, label %for.end
+
+for.end:                                          ; preds = %for.cond
+  ret void
+}
+