[SimplifyCFG] Create flag to disable simplifyCFG.

[morehouse]

Summary

When building with libFuzzer, simplifyCFG reduces the coverage signal
available to libFuzzer when trying to find new inputs. This patch
provides a way to disable simplifyCFG when building with libFuzzer.

Addresses https://llvm.org/pr36414.

[vitalybuka]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

109

why metadata instead of cl:opt like these?

[davide]

Some high level comments:

1. This is something that GCC does relatively frequently (adding frontend options to control optimization passes), but LLVM tends to not expose these details.

FWIW, I'd very much prefer the details of the optimizer wouldn't be exposed as frontend flags.

2. I really don't think metadata is the correct way of conveying this information to the optimizer. Among all the other problems, metadata can be stripped willy-nilly. Maybe a function attribute will work better?

Some alternatives which I don't like, but I can't currently propose anything better are:

1. Having a separate optimization pipeline that you use in these cases
2. Having an instrumentation pass that annotates your CFG to prevent passes from destroying coverage signal. That said, I'm not really familiar to comment on whether this is feasible or not.

Please note that completely disabling SimplifyCFG will result in non-trivial performance hits (at least, in my experience), so you might want to evaluate this carefully.
@chandlerc might have thoughts on how to address this.

[kcc]

We use function attributes in similar situations for asan/tsan/msan.
Similar, but not equivalent, so I don't know if we must follow the same pattern here.
The difference here is that we should keep the ability to turn optimization on and off from command line, regardless of what are the coverage instrumentation flags.

IIRC, the *function* metadata is not that easily stripped (as opposed to metadata attached to instructions).

[morehouse]

In D43423#1011170, @davide wrote:

Some high level comments:

1. This is something that GCC does relatively frequently (adding frontend options to control optimization passes), but LLVM tends to not expose these details.

FWIW, I'd very much prefer the details of the optimizer wouldn't be exposed as frontend flags.

We might not need the flag if we decide to always set no_simplify_cfg during codegen with coverage instrumentation. But we're not sure if we want to do that yet.

2. I really don't think metadata is the correct way of conveying this information to the optimizer. Among all the other problems, metadata can be stripped willy-nilly. Maybe a function attribute will work better?

Should be simple enough to change this to a function attribute if you think that is more appropriate. Wasn't sure whether metadata or an attribute made more sense.

Some alternatives which I don't like, but I can't currently propose anything better are:

1. Having a separate optimization pipeline that you use in these cases

This seems like a lot more work.

2. Having an instrumentation pass that annotates your CFG to prevent passes from destroying coverage signal. That said, I'm not really familiar to comment on whether this is feasible or not.

This patch allows us to annotate our functions with no_simplify_cfg to do what you're suggesting. Writing another instrumentation pass seems like overkill.

Please note that completely disabling SimplifyCFG will result in non-trivial performance hits (at least, in my experience), so you might want to evaluate this carefully.
@chandlerc might have thoughts on how to address this.

We're still evaluating this with mixed results. On one hand, disabling simplifyCFG makes most of our libFuzzer tests pass with -O2, and libFuzzer seems to perform at the same executions/sec rate. On the other hand, executions/sec doesn't necessarily translate to more coverage/sec or bugs found.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

109

We want to be able to avoid simplifyCFG when we pass -fsanitize=fuzzer to the Clang driver. AFAICT, the frontend does not explicitly invoke simplifyCFG, so it can't control the options here.

[efriedma]

FWIW, I'd very much prefer the details of the optimizer wouldn't be exposed as frontend flags.

Yes, definitely this. Frontend flags to control the optimizer should state an expected result, not just turn off some completely arbitrary set of transforms. Otherwise, we're likely to end up in a situation in the future where we add a new optimization, or split an existing pass, and it isn't clear which transforms the frontend flag should apply to.

[vitalybuka]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

6056

So maybe Attribute::SanitizeFuzzer for each function with --fsanitizer=fuzzer and then

if (... getFunction()->hasFnAttribute(Attribute::SanitizeFuzzer))
    return;

[vitalybuka]

I assume this is going to be abandoned in favor of D44057

clang/include/clang/Driver/CC1Options.td

HelpText<"Generate explicit import from anonymous namespace to containing"
            " scope">;
 def debug_forward_template_params : Flag<["-"], "debug-forward-template-params">,
   HelpText<"Emit complete descriptions of template parameters in forward"
            " declarations">;
 def fforbid_guard_variables : Flag<["-"], "fforbid-guard-variables">,
   HelpText<"Emit an error if a C++ static local initializer would need a guard variable">;
 def no_implicit_float : Flag<["-"], "no-implicit-float">,
   HelpText<"Don't generate implicit floating point instructions">;
+def fno_simplify_cfg : Flag<["-"], "fno-simplify-cfg">,
+  HelpText<"Disable CFG simplification">;
 def fdump_vtable_layouts : Flag<["-"], "fdump-vtable-layouts">,
   HelpText<"Dump the layouts of all vtables that will be emitted in a translation unit">;
 def fmerge_functions : Flag<["-"], "fmerge-functions">,
   HelpText<"Permit merging of identical functions when optimizing.">;
 def femit_coverage_notes : Flag<["-"], "femit-coverage-notes">,
   HelpText<"Emit a gcov coverage notes file when compiling.">;
 def femit_coverage_data: Flag<["-"], "femit-coverage-data">,
   HelpText<"Instrument the program to emit gcov coverage data when run.">;

clang/include/clang/Frontend/CodeGenOptions.def

 CODEGENOPT(NoCommon          , 1, 0) ///< Set when -fno-common or C++ is enabled.
 CODEGENOPT(NoDwarfDirectoryAsm , 1, 0) ///< Set when -fno-dwarf-directory-asm is
                                        ///< enabled.
 CODEGENOPT(NoExecStack       , 1, 0) ///< Set when -Wa,--noexecstack is enabled.
 CODEGENOPT(FatalWarnings     , 1, 0) ///< Set when -Wa,--fatal-warnings is
                                      ///< enabled.
 CODEGENOPT(EnableSegmentedStacks , 1, 0) ///< Set when -fsplit-stack is enabled.
 CODEGENOPT(NoImplicitFloat   , 1, 0) ///< Set when -mno-implicit-float is enabled.
+CODEGENOPT(NoSimplifyCFG     , 1, 0) ///< Disable CFG simplification.
 CODEGENOPT(NoInfsFPMath      , 1, 0) ///< Assume FP arguments, results not +-Inf.
 CODEGENOPT(NoSignedZeros     , 1, 0) ///< Allow ignoring the signedness of FP zero
 CODEGENOPT(Reassociate       , 1, 0) ///< Allow reassociation of FP math ops
 CODEGENOPT(ReciprocalMath    , 1, 0) ///< Allow FP divisions to be reassociated.
 CODEGENOPT(NoTrappingMath    , 1, 0) ///< Set when -fno-trapping-math is enabled.
 CODEGENOPT(NoNaNsFPMath      , 1, 0) ///< Assume FP arguments, results not NaN.
 CODEGENOPT(FlushDenorm       , 1, 0) ///< Allow FP denorm numbers to be flushed to zero
 CODEGENOPT(CorrectlyRoundedDivSqrt, 1, 0) ///< -cl-fp32-correctly-rounded-divide-sqrt

clang/lib/CodeGen/CodeGenFunction.cpp

if (SanOpts.hasOneOf(SanitizerKind::HWAddress))
     Fn->addFnAttr(llvm::Attribute::SanitizeHWAddress);
   if (SanOpts.has(SanitizerKind::Thread))
     Fn->addFnAttr(llvm::Attribute::SanitizeThread);
   if (SanOpts.has(SanitizerKind::Memory))
     Fn->addFnAttr(llvm::Attribute::SanitizeMemory);
   if (SanOpts.has(SanitizerKind::SafeStack))
     Fn->addFnAttr(llvm::Attribute::SafeStack);
 
+  // Apply no-simplify-cfg attribute to the function
+  if (CGM.getCodeGenOpts().NoSimplifyCFG)
+    Fn->setMetadata("no_simplify_cfg",
+                    llvm::MDNode::get(CGM.getLLVMContext(), None));
+
   // Ignore TSan memory acesses from within ObjC/ObjC++ dealloc, initialize,
   // .cxx_destruct, __destroy_helper_block_ and all of their calees at run time.
   if (SanOpts.has(SanitizerKind::Thread)) {
     if (const auto *OMD = dyn_cast_or_null<ObjCMethodDecl>(D)) {
       IdentifierInfo *II = OMD->getSelector().getIdentifierInfoForSlot(0);
       if (OMD->getMethodFamily() == OMF_dealloc ||
           OMD->getMethodFamily() == OMF_initialize ||
           (OMD->getSelector().isUnarySelector() && II->isStr(".cxx_destruct"))) {

clang/lib/Frontend/CompilerInvocation.cpp

Opts.NewStructPathTBAA = !Args.hasArg(OPT_no_struct_path_tbaa) &&
                            Args.hasArg(OPT_new_struct_path_tbaa);
   Opts.FineGrainedBitfieldAccesses =
       Args.hasFlag(OPT_ffine_grained_bitfield_accesses,
                    OPT_fno_fine_grained_bitfield_accesses, false);
   Opts.DwarfDebugFlags = Args.getLastArgValue(OPT_dwarf_debug_flags);
   Opts.MergeAllConstants = !Args.hasArg(OPT_fno_merge_all_constants);
   Opts.NoCommon = Args.hasArg(OPT_fno_common);
   Opts.NoImplicitFloat = Args.hasArg(OPT_no_implicit_float);
+  Opts.NoSimplifyCFG = Args.hasArg(OPT_fno_simplify_cfg);
   Opts.OptimizeSize = getOptimizationLevelSize(Args);
   Opts.SimplifyLibCalls = !(Args.hasArg(OPT_fno_builtin) ||
                             Args.hasArg(OPT_ffreestanding));
   if (Opts.SimplifyLibCalls)
     getAllNoBuiltinFuncValues(Args, Opts.NoBuiltinFuncs);
   Opts.UnrollLoops =
       Args.hasFlag(OPT_funroll_loops, OPT_fno_unroll_loops,
                    (Opts.OptimizationLevel > 1));

clang/test/CodeGen/no-simplify-cfg.cpp

+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s --check-prefix=SIMPLIFY
+// RUN: %clang_cc1 -fno-simplify-cfg -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: define i32 @_Z3foov() {{.*}} !no_simplify_cfg
+int foo() {
+  return 42;
+}
+
+// CHECK: define i32 @_Z3barv() {{.*}} !no_simplify_cfg
+int bar() {
+  return foo() * foo() + foo();
+}
+
+// CHECK: define i32 @_Z3bazii(i32 %x, i32 %y) {{.*}} !no_simplify_cfg
+int baz(int x, int y) {
+  int z = x / y;
+  return z + foo() - bar();
+}
+
+// SIMPLIFY-NOT: no_simplify_cfg

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

 static cl::opt<bool>
     SinkCommon("simplifycfg-sink-common", cl::Hidden, cl::init(true),
                cl::desc("Sink common instructions down to the end block"));
 
 static cl::opt<bool> HoistCondStores(
     "simplifycfg-hoist-cond-stores", cl::Hidden, cl::init(true),
     cl::desc("Hoist conditional stores if an unconditional store precedes"));
 
 static cl::opt<bool> MergeCondStores(
     "simplifycfg-merge-cond-stores", cl::Hidden, cl::init(true),
     cl::desc("Hoist conditional stores even if an unconditional store does not "
              "precede - hoist multiple conditional stores into a single "
              "predicated store"));
 
 static cl::opt<bool> MergeCondStoresAggressively(
     "simplifycfg-merge-cond-stores-aggressively", cl::Hidden, cl::init(false),
     cl::desc("When merging conditional stores, do so even if the resultant "
bool SimplifyCFGOpt::run(BasicBlock *BB) {
   }
 
   return Changed;
 }
 
 bool llvm::simplifyCFG(BasicBlock *BB, const TargetTransformInfo &TTI,
                        const SimplifyCFGOptions &Options,
                        SmallPtrSetImpl<BasicBlock *> *LoopHeaders) {
+  const Function *Fn = BB->getParent();
+  if (Fn && Fn->getMetadata("no_simplify_cfg"))
+    return false;
   return SimplifyCFGOpt(TTI, BB->getModule()->getDataLayout(), LoopHeaders,
                         Options)
       .run(BB);
 }

llvm/test/Transforms/SimplifyCFG/no-simplify-cfg.ll

+; RUN: opt < %s -simplifycfg -S | FileCheck %s
+
+define i32 @foo(i32 %x) !no_simplify_cfg !{} {
+entry:
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %0 = load i32, i32* %x.addr, align 4
+  %cmp = icmp sgt i32 %0, 16
+  br i1 %cmp, label %land.rhs, label %land.end
+
+land.rhs:
+  %1 = load i32, i32* %x.addr, align 4
+  %cmp1 = icmp slt i32 %1, 32
+  br label %land.end
+
+land.end:
+  %2 = phi i1 [ false, %entry ], [ %cmp1, %land.rhs ]
+  %conv = zext i1 %2 to i32
+  ret i32 %conv
+
+; CHECK-LABEL: define i32 @foo(i32 %x) !no_simplify_cfg
+; CHECK-LABEL: entry:
+; CHECK: br i1 %cmp, label %land.rhs, label %land.end
+; CHECK-LABEL: land.rhs:
+; CHECK: br label %land.end
+; CHECK-LABEL: land.end:
+; CHECK: phi {{.*}} %entry {{.*}} %land.rhs
+}
+
+define i32 @bar(i32 %x) {
+entry:
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %0 = load i32, i32* %x.addr, align 4
+  %cmp = icmp sgt i32 %0, 16
+  br i1 %cmp, label %land.rhs, label %land.end
+
+land.rhs:
+  %1 = load i32, i32* %x.addr, align 4
+  %cmp1 = icmp slt i32 %1, 32
+  br label %land.end
+
+land.end:
+  %2 = phi i1 [ false, %entry ], [ %cmp1, %land.rhs ]
+  %conv = zext i1 %2 to i32
+  ret i32 %conv
+
+; CHECK-LABEL: define i32 @bar(i32 %x)
+; CHECK-NOT: br
+}