Revert "SimplifyCFG: Clean up optforfuzzing implementation"

[zequanwu]

Summary

In commit cdd006eec9409923f9a56b9026ce2cb72e7b71dc, the options SimplifyCondBranch and FoldTwoEntryPHINode were added and set to false when attribute OptForFuzzing is present. We also need to do the same for SimplifyCFGPass which is NPM version of CFGSimplifyPass. It will be duplicated. We can just check for the attribute at simplifyCondBranch and FoldTwoEntryPHINode
See discussion: https://reviews.llvm.org/D89590
This reverts commit cdd006eec9409923f9a56b9026ce2cb72e7b71dc.

[aeubanks]

lgtm, but I'll let arsenm approve

[arsenm]

I disagree with this change. SimplifyCFG should continue to pass in options for transform options as is done for the others. I don't think avoiding duplication between the new PM and code that will be deleted is worth it

[aeubanks]

https://reviews.llvm.org/D44232 makes it sound like we always want to disable certain optimizations when optforfuzzing is specified, so the checks should be done in the transform, not the pass setup. We can have the options you introduced to disable those optimizations no matter what (as I suggested in https://reviews.llvm.org/D89590), but I think we should at least always check for optforfuzzing in SimplifyCFG.cpp. Does that make sense?

[lebedev.ri]

Thank you for the patch!

In D89917#2346037, @arsenm wrote:

I disagree with this change. SimplifyCFG should continue to pass in options for transform options as is done for the others. I don't think avoiding duplication between the new PM and code that will be deleted is worth it

I disagree here. Why is that beneficial, if we want to force-disable those transforms regardless of their previous configured status?

[arsenm]

In D89917#2346421, @lebedev.ri wrote:

Thank you for the patch!

In D89917#2346037, @arsenm wrote:

I disagree with this change. SimplifyCFG should continue to pass in options for transform options as is done for the others. I don't think avoiding duplication between the new PM and code that will be deleted is worth it

I disagree here. Why is that beneficial, if we want to force-disable those transforms regardless of their previous configured status?

Because SimplifyCFG is a standalone utility, not just a pass. It's a pass's responsibility to ensure attributes are respected, not individual transformations. If I'm writing another control flow pass (e.g. a structurizer) and calling these utilities, the transformation options should not be limited based on this extremely poorly specified attribute.

[lebedev.ri]

In D89917#2347088, @arsenm wrote:

In D89917#2346421, @lebedev.ri wrote:

Thank you for the patch!

In D89917#2346037, @arsenm wrote:

I disagree with this change. SimplifyCFG should continue to pass in options for transform options as is done for the others. I don't think avoiding duplication between the new PM and code that will be deleted is worth it

I disagree here. Why is that beneficial, if we want to force-disable those transforms regardless of their previous configured status?

Because SimplifyCFG is a standalone utility, not just a pass. It's a pass's responsibility to ensure attributes are respected, not individual transformations. If I'm writing another control flow pass (e.g. a structurizer) and calling these utilities, the transformation options should not be limited based on this extremely poorly specified attribute.

Let's back-track here.
Are you saying that those folds should not disable themselves in presence of OptForFuzzing attribute,
or that they should be guarded by their options?
Because i agree with later, but disagree with former.

[arsenm]

In D89917#2347182, @lebedev.ri wrote:

In D89917#2347088, @arsenm wrote:

In D89917#2346421, @lebedev.ri wrote:

Thank you for the patch!

In D89917#2346037, @arsenm wrote:

I disagree with this change. SimplifyCFG should continue to pass in options for transform options as is done for the others. I don't think avoiding duplication between the new PM and code that will be deleted is worth it

I disagree here. Why is that beneficial, if we want to force-disable those transforms regardless of their previous configured status?

Because SimplifyCFG is a standalone utility, not just a pass. It's a pass's responsibility to ensure attributes are respected, not individual transformations. If I'm writing another control flow pass (e.g. a structurizer) and calling these utilities, the transformation options should not be limited based on this extremely poorly specified attribute.

Let's back-track here.
Are you saying that those folds should not disable themselves in presence of OptForFuzzing attribute,
or that they should be guarded by their options?
Because i agree with later, but disagree with former.

I'm saying these folds should be guarded by the SimplifyCFGOptions flags. The pass itself should initialize the option field based on the attribute. Other uses of SimplifyCFG the utilities can consider the attribute at their discretion

[aeubanks]

+morehouse who added optforfuzzing

[aeubanks]

morehouse, any thoughts on this?

[morehouse]

I'm not familiar with users of standalone SimplifyCFG.

My primary concern is to keep these optimizations disabled under -fsanitize=fuzzer since they reduce the coverage signal we get from SanCov. AFAIU, either approach works for this.

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp

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);
     return simplifyFunctionCFG(F, TTI, Options);
   }
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addRequired<AssumptionCacheTracker>();
     AU.addRequired<TargetTransformInfoWrapperPass>();
     AU.addPreserved<GlobalsAAWrapperPass>();
   }

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))
     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())
bool SimplifyCFGOpt::simplifyOnce(BasicBlock *BB) {
   if (MergeBlockIntoPredecessor(BB))
     return true;
 
   if (SinkCommon && Options.SinkCommonInsts)
     Changed |= SinkCommonCodeFromPredecessors(BB);
 
   IRBuilder<> Builder(BB);
 
-  if (Options.FoldTwoEntryPHINode) {
-    // If there is a trivial two-entry PHI node in this basic block, and we can
-    // eliminate it, do so now.
-    if (auto *PN = dyn_cast<PHINode>(BB->begin()))
-      if (PN->getNumIncomingValues() == 2)
-        Changed |= FoldTwoEntryPHINode(PN, TTI, DL);
-  }
 
   Instruction *Terminator = BB->getTerminator();
   Builder.SetInsertPoint(Terminator);
   switch (Terminator->getOpcode()) {
   case Instruction::Br:
     Changed |= simplifyBranch(cast<BranchInst>(Terminator), Builder);
     break;
   case Instruction::Ret: