[NPM][SimplifyCFGPass] For OptForFuzzing attribute, disable SimplifyCondBranch and FoldTwoEntryPHINode in NPM

[zequanwu]

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp

280

Copied from here.

[lebedev.ri]

This is missing description and test coverage.
Why is this NPM specific?
The specific folds should be guarded with that F.hasFnAttribute(Attribute::OptForFuzzing) check i think.

[aeubanks]

Can you add a corresponding -passes=simplifycfg RUN line in the failing test?

[aeubanks]

In D89590#2335840, @lebedev.ri wrote:

Why is this NPM specific?

This is something the legacy pass did but was not properly ported to the NPM pass.

[zequanwu]

update test.

[lebedev.ri]

In D89590#2335864, @aeubanks wrote:

In D89590#2335840, @lebedev.ri wrote:

Why is this NPM specific?

This is something the legacy pass did but was not properly ported to the NPM pass.

Right. But why is this PM-specific at all, why aren't the folds themselves guarded with that?

[aeubanks]

In D89590#2335885, @lebedev.ri wrote:

In D89590#2335864, @aeubanks wrote:

In D89590#2335840, @lebedev.ri wrote:

Why is this NPM specific?

This is something the legacy pass did but was not properly ported to the NPM pass.

Right. But why is this PM-specific at all, why aren't the folds themselves guarded with that?

Oh I see, maybe we don't even need SimplifyCondBranch and FoldTwoEntryPHINode in SimplifyCFGOptions, just check for the OptForFuzzing attribute at time of use.

[zequanwu]

In D89590#2335885, @lebedev.ri wrote:

In D89590#2335864, @aeubanks wrote:

In D89590#2335840, @lebedev.ri wrote:

Why is this NPM specific?

This is something the legacy pass did but was not properly ported to the NPM pass.

Right. But why is this PM-specific at all, why aren't the folds themselves guarded with that?

It's from here: https://github.com/llvm/llvm-project/commit/cdd006eec9409923f9a56b9026ce2cb72e7b71dc. It was guarded by FoldTwoEntryPHINode and simplifyCondBranch.
As the commit message said, "This should function as any other SimplifyCFGOption rather than having
the transform check and specially consider the attribute itself.". I think it was intended to have SimplifyCFGOptions as a control for SimplifyCondBranch and FoldTwoEntryPHINode.

[lebedev.ri]

In D89590#2335917, @aeubanks wrote:

In D89590#2335885, @lebedev.ri wrote:

In D89590#2335864, @aeubanks wrote:

In D89590#2335840, @lebedev.ri wrote:

Why is this NPM specific?

This is something the legacy pass did but was not properly ported to the NPM pass.

Right. But why is this PM-specific at all, why aren't the folds themselves guarded with that?

Oh I see, maybe we don't even need SimplifyCondBranch and FoldTwoEntryPHINode in SimplifyCFGOptions, just check for the OptForFuzzing attribute at time of use.

I think the transforms themselves should be explicitly guarded with !F.hasFnAttribute(Attribute::OptForFuzzing),
but i don't feel strongly about it..

[aeubanks]

I'd say either revert https://github.com/llvm/llvm-project/commit/cdd006eec9409923f9a56b9026ce2cb72e7b71dc, or in FoldTwoEntryPHINode() check that the option is set and that the OptForFuzzing attribute is not set (same with simplifyCondBranch()). I guess I'd slightly prefer the second option as to not totally revert the original commit.

[zequanwu]

In D89590#2342497, @aeubanks wrote:

I'd say either revert https://github.com/llvm/llvm-project/commit/cdd006eec9409923f9a56b9026ce2cb72e7b71dc, or in FoldTwoEntryPHINode() check that the option is set and that the OptForFuzzing attribute is not set (same with simplifyCondBranch()). I guess I'd slightly prefer the second option as to not totally revert the original commit.

Why do we want to check the option and not OptForFuzzing? Doesn't they have the same truth value?

[aeubanks]

In D89590#2343196, @zequanwu wrote:

In D89590#2342497, @aeubanks wrote:

I'd say either revert https://github.com/llvm/llvm-project/commit/cdd006eec9409923f9a56b9026ce2cb72e7b71dc, or in FoldTwoEntryPHINode() check that the option is set and that the OptForFuzzing attribute is not set (same with simplifyCondBranch()). I guess I'd slightly prefer the second option as to not totally revert the original commit.

Why do we want to check the option and not OptForFuzzing? Doesn't they have the same truth value?

As of right now, yes, but the point of https://github.com/llvm/llvm-project/commit/cdd006eec9409923f9a56b9026ce2cb72e7b71dc was that in the future, some consumer of simplifyCFG() may want to set those options. I'm not against a complete revert of that change, but it does sort of make sense.

[zequanwu]

address comment

[aeubanks]

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp

248–251

the changes in this file shouldn't be necessary any more

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

2419

Fn should never be null, ditto below

[zequanwu]

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp

248–251

Then this is pretty much the same as reverting the original commit. And Line 280-286 wouldn't be necessary either (LPM pass).

[aeubanks]

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp

248–251

This would allow turning off SimplifyCondBranch and FoldTwoEntryPHINode, but otherwise yes. Maybe we should just revert with arsenm's approval.

[zequanwu]

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp

248–251

I add a revert patch here: D89917

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp

SimplifyCFGPass::SimplifyCFGPass(const SimplifyCFGOptions &Opts)
     : Options(Opts) {
   applyCommandLineOverridesToOptions(Options);
 }
 
 PreservedAnalyses SimplifyCFGPass::run(Function &F,
                                        FunctionAnalysisManager &AM) {
   auto &TTI = AM.getResult<TargetIRAnalysis>(F);
   Options.AC = &AM.getResult<AssumptionAnalysis>(F);
+  if (F.hasFnAttribute(Attribute::OptForFuzzing))
+    Options.setSimplifyCondBranch(false).setFoldTwoEntryPHINode(false);
+  else
+    Options.setSimplifyCondBranch(true).setFoldTwoEntryPHINode(true);
   if (!simplifyFunctionCFG(F, TTI, Options))
     return PreservedAnalyses::all();
   PreservedAnalyses PA;
   PA.preserve<GlobalsAA>();
   return PA;
 }
 
 namespace {
CFGSimplifyPass(SimplifyCFGOptions Options_ = SimplifyCFGOptions(),
     applyCommandLineOverridesToOptions(Options);
   }
 
   bool runOnFunction(Function &F) override {
     if (skipFunction(F) || (PredicateFtor && !PredicateFtor(F)))
       return false;
 
     Options.AC = &getAnalysis<AssumptionCacheTracker>().getAssumptionCache(F);
     if (F.hasFnAttribute(Attribute::OptForFuzzing)) {
       Options.setSimplifyCondBranch(false)
              .setFoldTwoEntryPHINode(false);
     } else {
       Options.setSimplifyCondBranch(true)
              .setFoldTwoEntryPHINode(true);
     }
 
     auto &TTI = getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

static bool FoldTwoEntryPHINode(PHINode *PN, const TargetTransformInfo &TTI,
                                 const DataLayout &DL) {
   // Ok, this is a two entry PHI node.  Check to see if this is a simple "if
   // statement", which has a very simple dominance structure.  Basically, we
   // are trying to find the condition that is being branched on, which
   // subsequently causes this merge to happen.  We really want control
   // dependence information for this check, but simplifycfg can't keep it up
   // to date, and this catches most of the cases we care about anyway.
   BasicBlock *BB = PN->getParent();
+  const Function *Fn = BB->getParent();
+  if (Fn && Fn->hasFnAttribute(Attribute::OptForFuzzing))
+    return false;
 
   BasicBlock *IfTrue, *IfFalse;
   Value *IfCond = GetIfCondition(BB, IfTrue, IfFalse);
   if (!IfCond ||
       // Don't bother if the branch will be constant folded trivially.
       isa<ConstantInt>(IfCond))
     return false;
 
if (!PPred || (PredPred && PredPred != PPred))
       return nullptr;
     PredPred = PPred;
   }
   return PredPred;
 }
 
 bool SimplifyCFGOpt::simplifyCondBranch(BranchInst *BI, IRBuilder<> &Builder) {
   BasicBlock *BB = BI->getParent();
-  if (!Options.SimplifyCondBranch)
+  const Function *Fn = BB->getParent();
+  if (Fn && Fn->hasFnAttribute(Attribute::OptForFuzzing) &&
+      !Options.SimplifyCondBranch)
     return false;
 
   // Conditional branch
   if (isValueEqualityComparison(BI)) {
     // If we only have one predecessor, and if it is a branch on this value,
     // see if that predecessor totally determines the outcome of this
     // switch.
     if (BasicBlock *OnlyPred = BB->getSinglePredecessor())

llvm/test/Transforms/SimplifyCFG/opt-for-fuzzing.ll

 ; RUN: opt < %s -simplifycfg -S | FileCheck %s
+; RUN: opt < %s -passes=simplifycfg -S | FileCheck %s
 
 define i32 @foo(i32 %x) optforfuzzing {
 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