[llvm] 1067d3e - Revert "[NFCI] createCFGSimplificationPass(): migrate to also take SimplifyCFGOptions"
Roman Lebedev via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 16 02:51:52 PDT 2020
Or better question, is it enough to simply drop include of
SimplifyCFGOptions.h from Local.h ?
On Thu, Jul 16, 2020 at 12:47 PM Roman Lebedev <lebedev.ri at gmail.com> wrote:
>
> Thanks for the reverts. I have not received any notifications
> about failed builds from bots. On which bots did this fail?
>
> Unfortunately, these weren't just passing-by refactorings.
> The current status-quo of having those multi-param constructors
> is really worrying. In particular, in a follow-up patch i need to
> add one more parameter there, and let's just say it was not pleasant to do :)
>
> So let's see how this can be laid out alternatively.
> What if SimplifyCFGOptions.h goes into Transforms/Utils/ ?
>
> Roman
>
> On Thu, Jul 16, 2020 at 11:58 AM Adrian Kuegel via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> >
> >
> > Author: Adrian Kuegel
> > Date: 2020-07-16T10:54:10+02:00
> > New Revision: 1067d3e176ea7b0b1942c163bf8c6c90107768c1
> >
> > URL: https://github.com/llvm/llvm-project/commit/1067d3e176ea7b0b1942c163bf8c6c90107768c1
> > DIFF: https://github.com/llvm/llvm-project/commit/1067d3e176ea7b0b1942c163bf8c6c90107768c1.diff
> >
> > LOG: Revert "[NFCI] createCFGSimplificationPass(): migrate to also take SimplifyCFGOptions"
> >
> > This reverts commit b2018198c32a0535bb1f5bb5b40fbcf50d8d47b7.
> > This commit introduced a Dependency Cycle between Transforms/Scalar and
> > Transforms/Utils. Transforms/Scalar already depends on Transforms/Utils,
> > so if SimplifyCFGOptions.h is moved to Scalar, and Utils/Local.h still
> > depends on it, we have a cycle.
> >
> > Added:
> >
> >
> > Modified:
> > llvm/include/llvm/Transforms/Scalar.h
> > llvm/include/llvm/Transforms/Scalar/SimplifyCFG.h
> > llvm/include/llvm/Transforms/Utils/Local.h
> > llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
> > llvm/lib/Target/ARM/ARMTargetMachine.cpp
> > llvm/lib/Target/Hexagon/HexagonTargetMachine.cpp
> > llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
> > llvm/lib/Transforms/Scalar/Scalar.cpp
> > llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
> >
> > Removed:
> > llvm/include/llvm/Transforms/Scalar/SimplifyCFGOptions.h
> >
> >
> > ################################################################################
> > diff --git a/llvm/include/llvm/Transforms/Scalar.h b/llvm/include/llvm/Transforms/Scalar.h
> > index 19d158a2a1b5..a1aacec76979 100644
> > --- a/llvm/include/llvm/Transforms/Scalar.h
> > +++ b/llvm/include/llvm/Transforms/Scalar.h
> > @@ -14,7 +14,6 @@
> > #ifndef LLVM_TRANSFORMS_SCALAR_H
> > #define LLVM_TRANSFORMS_SCALAR_H
> >
> > -#include "llvm/Transforms/Scalar/SimplifyCFGOptions.h"
> > #include <functional>
> >
> > namespace llvm {
> > @@ -257,7 +256,8 @@ FunctionPass *createJumpThreadingPass(int Threshold = -1);
> > // simplify terminator instructions, convert switches to lookup tables, etc.
> > //
> > FunctionPass *createCFGSimplificationPass(
> > - SimplifyCFGOptions Options = SimplifyCFGOptions(),
> > + unsigned Threshold = 1, bool ForwardSwitchCond = false,
> > + bool ConvertSwitch = false, bool KeepLoops = true, bool SinkCommon = false,
> > std::function<bool(const Function &)> Ftor = nullptr);
> >
> > //===----------------------------------------------------------------------===//
> >
> > diff --git a/llvm/include/llvm/Transforms/Scalar/SimplifyCFG.h b/llvm/include/llvm/Transforms/Scalar/SimplifyCFG.h
> > index 026c183ec891..f9792d38bbe6 100644
> > --- a/llvm/include/llvm/Transforms/Scalar/SimplifyCFG.h
> > +++ b/llvm/include/llvm/Transforms/Scalar/SimplifyCFG.h
> > @@ -14,9 +14,9 @@
> > #ifndef LLVM_TRANSFORMS_SCALAR_SIMPLIFYCFG_H
> > #define LLVM_TRANSFORMS_SCALAR_SIMPLIFYCFG_H
> >
> > +#include "llvm/Transforms/Utils/Local.h"
> > #include "llvm/IR/Function.h"
> > #include "llvm/IR/PassManager.h"
> > -#include "llvm/Transforms/Scalar/SimplifyCFGOptions.h"
> >
> > namespace llvm {
> >
> >
> > diff --git a/llvm/include/llvm/Transforms/Scalar/SimplifyCFGOptions.h b/llvm/include/llvm/Transforms/Scalar/SimplifyCFGOptions.h
> > deleted file mode 100644
> > index 42df3af5d747..000000000000
> > --- a/llvm/include/llvm/Transforms/Scalar/SimplifyCFGOptions.h
> > +++ /dev/null
> > @@ -1,86 +0,0 @@
> > -//===- SimplifyCFGOptions.h - Control structure for SimplifyCFG -*- C++ -*-===//
> > -//
> > -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
> > -// See https://llvm.org/LICENSE.txt for license information.
> > -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
> > -//
> > -//===----------------------------------------------------------------------===//
> > -//
> > -// A set of parameters used to control the transforms in the SimplifyCFG pass.
> > -// Options may change depending on the position in the optimization pipeline.
> > -// For example, canonical form that includes switches and branches may later be
> > -// replaced by lookup tables and selects.
> > -//
> > -//===----------------------------------------------------------------------===//
> > -
> > -#ifndef LLVM_TRANSFORMS_SCALAR_SIMPLIFYCFGOPTIONS_H
> > -#define LLVM_TRANSFORMS_SCALAR_SIMPLIFYCFGOPTIONS_H
> > -
> > -namespace llvm {
> > -
> > -class AssumptionCache;
> > -
> > -struct SimplifyCFGOptions {
> > - int BonusInstThreshold;
> > - bool ForwardSwitchCondToPhi;
> > - bool ConvertSwitchToLookupTable;
> > - bool NeedCanonicalLoop;
> > - bool SinkCommonInsts;
> > - bool SimplifyCondBranch;
> > - bool FoldTwoEntryPHINode;
> > -
> > - AssumptionCache *AC;
> > -
> > - SimplifyCFGOptions(unsigned BonusThreshold = 1,
> > - bool ForwardSwitchCond = false,
> > - bool SwitchToLookup = false, bool CanonicalLoops = true,
> > - bool SinkCommon = false,
> > - AssumptionCache *AssumpCache = nullptr,
> > - bool SimplifyCondBranch = true,
> > - bool FoldTwoEntryPHINode = true)
> > - : BonusInstThreshold(BonusThreshold),
> > - ForwardSwitchCondToPhi(ForwardSwitchCond),
> > - ConvertSwitchToLookupTable(SwitchToLookup),
> > - NeedCanonicalLoop(CanonicalLoops), SinkCommonInsts(SinkCommon),
> > - SimplifyCondBranch(SimplifyCondBranch),
> > - FoldTwoEntryPHINode(FoldTwoEntryPHINode), AC(AssumpCache) {}
> > -
> > - // Support 'builder' pattern to set members by name at construction time.
> > - SimplifyCFGOptions &bonusInstThreshold(int I) {
> > - BonusInstThreshold = I;
> > - return *this;
> > - }
> > - SimplifyCFGOptions &forwardSwitchCondToPhi(bool B) {
> > - ForwardSwitchCondToPhi = B;
> > - return *this;
> > - }
> > - SimplifyCFGOptions &convertSwitchToLookupTable(bool B) {
> > - ConvertSwitchToLookupTable = B;
> > - return *this;
> > - }
> > - SimplifyCFGOptions &needCanonicalLoops(bool B) {
> > - NeedCanonicalLoop = B;
> > - return *this;
> > - }
> > - SimplifyCFGOptions &sinkCommonInsts(bool B) {
> > - SinkCommonInsts = B;
> > - return *this;
> > - }
> > - SimplifyCFGOptions &setAssumptionCache(AssumptionCache *Cache) {
> > - AC = Cache;
> > - return *this;
> > - }
> > - SimplifyCFGOptions &setSimplifyCondBranch(bool B) {
> > - SimplifyCondBranch = B;
> > - return *this;
> > - }
> > -
> > - SimplifyCFGOptions &setFoldTwoEntryPHINode(bool B) {
> > - FoldTwoEntryPHINode = B;
> > - return *this;
> > - }
> > -};
> > -
> > -} // namespace llvm
> > -
> > -#endif // LLVM_TRANSFORMS_SCALAR_SIMPLIFYCFGOPTIONS_H
> >
> > diff --git a/llvm/include/llvm/Transforms/Utils/Local.h b/llvm/include/llvm/Transforms/Utils/Local.h
> > index 3595dd627d5b..3fab3bc21a07 100644
> > --- a/llvm/include/llvm/Transforms/Utils/Local.h
> > +++ b/llvm/include/llvm/Transforms/Utils/Local.h
> > @@ -30,7 +30,6 @@
> > #include "llvm/IR/Value.h"
> > #include "llvm/IR/ValueHandle.h"
> > #include "llvm/Support/Casting.h"
> > -#include "llvm/Transforms/Scalar/SimplifyCFGOptions.h"
> > #include <cstdint>
> > #include <limits>
> >
> > @@ -59,6 +58,73 @@ class StoreInst;
> > class TargetLibraryInfo;
> > class TargetTransformInfo;
> >
> > +/// A set of parameters used to control the transforms in the SimplifyCFG pass.
> > +/// Options may change depending on the position in the optimization pipeline.
> > +/// For example, canonical form that includes switches and branches may later be
> > +/// replaced by lookup tables and selects.
> > +struct SimplifyCFGOptions {
> > + int BonusInstThreshold;
> > + bool ForwardSwitchCondToPhi;
> > + bool ConvertSwitchToLookupTable;
> > + bool NeedCanonicalLoop;
> > + bool SinkCommonInsts;
> > + bool SimplifyCondBranch;
> > + bool FoldTwoEntryPHINode;
> > +
> > + AssumptionCache *AC;
> > +
> > + SimplifyCFGOptions(unsigned BonusThreshold = 1,
> > + bool ForwardSwitchCond = false,
> > + bool SwitchToLookup = false, bool CanonicalLoops = true,
> > + bool SinkCommon = false,
> > + AssumptionCache *AssumpCache = nullptr,
> > + bool SimplifyCondBranch = true,
> > + bool FoldTwoEntryPHINode = true)
> > + : BonusInstThreshold(BonusThreshold),
> > + ForwardSwitchCondToPhi(ForwardSwitchCond),
> > + ConvertSwitchToLookupTable(SwitchToLookup),
> > + NeedCanonicalLoop(CanonicalLoops),
> > + SinkCommonInsts(SinkCommon),
> > + SimplifyCondBranch(SimplifyCondBranch),
> > + FoldTwoEntryPHINode(FoldTwoEntryPHINode),
> > + AC(AssumpCache) {}
> > +
> > + // Support 'builder' pattern to set members by name at construction time.
> > + SimplifyCFGOptions &bonusInstThreshold(int I) {
> > + BonusInstThreshold = I;
> > + return *this;
> > + }
> > + SimplifyCFGOptions &forwardSwitchCondToPhi(bool B) {
> > + ForwardSwitchCondToPhi = B;
> > + return *this;
> > + }
> > + SimplifyCFGOptions &convertSwitchToLookupTable(bool B) {
> > + ConvertSwitchToLookupTable = B;
> > + return *this;
> > + }
> > + SimplifyCFGOptions &needCanonicalLoops(bool B) {
> > + NeedCanonicalLoop = B;
> > + return *this;
> > + }
> > + SimplifyCFGOptions &sinkCommonInsts(bool B) {
> > + SinkCommonInsts = B;
> > + return *this;
> > + }
> > + SimplifyCFGOptions &setAssumptionCache(AssumptionCache *Cache) {
> > + AC = Cache;
> > + return *this;
> > + }
> > + SimplifyCFGOptions &setSimplifyCondBranch(bool B) {
> > + SimplifyCondBranch = B;
> > + return *this;
> > + }
> > +
> > + SimplifyCFGOptions &setFoldTwoEntryPHINode(bool B) {
> > + FoldTwoEntryPHINode = B;
> > + return *this;
> > + }
> > +};
> > +
> > //===----------------------------------------------------------------------===//
> > // Local constant propagation.
> > //
> >
> > diff --git a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
> > index b0cef9b66e01..a63b9a97ada5 100644
> > --- a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
> > +++ b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
> > @@ -453,11 +453,7 @@ void AArch64PassConfig::addIRPasses() {
> > // determine whether it succeeded. We can exploit existing control-flow in
> > // ldrex/strex loops to simplify this, but it needs tidying up.
> > if (TM->getOptLevel() != CodeGenOpt::None && EnableAtomicTidy)
> > - addPass(createCFGSimplificationPass(SimplifyCFGOptions()
> > - .forwardSwitchCondToPhi(true)
> > - .convertSwitchToLookupTable(true)
> > - .needCanonicalLoops(false)
> > - .sinkCommonInsts(true)));
> > + addPass(createCFGSimplificationPass(1, true, true, false, true));
> >
> > // Run LoopDataPrefetch
> > //
> >
> > diff --git a/llvm/lib/Target/ARM/ARMTargetMachine.cpp b/llvm/lib/Target/ARM/ARMTargetMachine.cpp
> > index b316b1041f2c..9ead5fa4308c 100644
> > --- a/llvm/lib/Target/ARM/ARMTargetMachine.cpp
> > +++ b/llvm/lib/Target/ARM/ARMTargetMachine.cpp
> > @@ -409,7 +409,7 @@ void ARMPassConfig::addIRPasses() {
> > // ldrex/strex loops to simplify this, but it needs tidying up.
> > if (TM->getOptLevel() != CodeGenOpt::None && EnableAtomicTidy)
> > addPass(createCFGSimplificationPass(
> > - SimplifyCFGOptions().sinkCommonInsts(true), [this](const Function &F) {
> > + 1, false, false, true, true, [this](const Function &F) {
> > const auto &ST = this->TM->getSubtarget<ARMSubtarget>(F);
> > return ST.hasAnyDataBarrier() && !ST.isThumb1Only();
> > }));
> >
> > diff --git a/llvm/lib/Target/Hexagon/HexagonTargetMachine.cpp b/llvm/lib/Target/Hexagon/HexagonTargetMachine.cpp
> > index 49d98622d946..3fe42ea13f51 100644
> > --- a/llvm/lib/Target/Hexagon/HexagonTargetMachine.cpp
> > +++ b/llvm/lib/Target/Hexagon/HexagonTargetMachine.cpp
> > @@ -320,11 +320,7 @@ void HexagonPassConfig::addIRPasses() {
> >
> > if (!NoOpt) {
> > if (EnableInitialCFGCleanup)
> > - addPass(createCFGSimplificationPass(SimplifyCFGOptions()
> > - .forwardSwitchCondToPhi(true)
> > - .convertSwitchToLookupTable(true)
> > - .needCanonicalLoops(false)
> > - .sinkCommonInsts(true)));
> > + addPass(createCFGSimplificationPass(1, true, true, false, true));
> > if (EnableLoopPrefetch)
> > addPass(createLoopDataPrefetchPass());
> > if (EnableCommGEP)
> >
> > diff --git a/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp b/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
> > index 460297a26020..d73d42c52074 100644
> > --- a/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
> > +++ b/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
> > @@ -777,11 +777,7 @@ void PassManagerBuilder::populateModulePassManager(
> > // convert to more optimized IR using more aggressive simplify CFG options.
> > // The extra sinking transform can create larger basic blocks, so do this
> > // before SLP vectorization.
> > - MPM.add(createCFGSimplificationPass(SimplifyCFGOptions()
> > - .forwardSwitchCondToPhi(true)
> > - .convertSwitchToLookupTable(true)
> > - .needCanonicalLoops(false)
> > - .sinkCommonInsts(true)));
> > + MPM.add(createCFGSimplificationPass(1, true, true, false, true));
> >
> > if (SLPVectorize) {
> > MPM.add(createSLPVectorizerPass()); // Vectorize parallel scalar chains.
> >
> > diff --git a/llvm/lib/Transforms/Scalar/Scalar.cpp b/llvm/lib/Transforms/Scalar/Scalar.cpp
> > index 42f79d89f0a2..9d088547b436 100644
> > --- a/llvm/lib/Transforms/Scalar/Scalar.cpp
> > +++ b/llvm/lib/Transforms/Scalar/Scalar.cpp
> > @@ -139,7 +139,7 @@ void LLVMAddAlignmentFromAssumptionsPass(LLVMPassManagerRef PM) {
> > }
> >
> > void LLVMAddCFGSimplificationPass(LLVMPassManagerRef PM) {
> > - unwrap(PM)->add(createCFGSimplificationPass());
> > + unwrap(PM)->add(createCFGSimplificationPass(1, false, false, true));
> > }
> >
> > void LLVMAddDeadStoreEliminationPass(LLVMPassManagerRef PM) {
> >
> > diff --git a/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp b/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
> > index f4ed24c92bf2..4187d5b55adf 100644
> > --- a/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
> > +++ b/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
> > @@ -39,7 +39,6 @@
> > #include "llvm/Support/CommandLine.h"
> > #include "llvm/Transforms/Scalar.h"
> > #include "llvm/Transforms/Scalar/SimplifyCFG.h"
> > -#include "llvm/Transforms/Scalar/SimplifyCFGOptions.h"
> > #include "llvm/Transforms/Utils/Local.h"
> > #include <utility>
> > using namespace llvm;
> > @@ -305,7 +304,15 @@ INITIALIZE_PASS_END(CFGSimplifyPass, "simplifycfg", "Simplify the CFG", false,
> >
> > // Public interface to the CFGSimplification pass
> > FunctionPass *
> > -llvm::createCFGSimplificationPass(SimplifyCFGOptions Options,
> > +llvm::createCFGSimplificationPass(unsigned Threshold, bool ForwardSwitchCond,
> > + bool ConvertSwitch, bool KeepLoops,
> > + bool SinkCommon,
> > std::function<bool(const Function &)> Ftor) {
> > - return new CFGSimplifyPass(Options, std::move(Ftor));
> > + return new CFGSimplifyPass(SimplifyCFGOptions()
> > + .bonusInstThreshold(Threshold)
> > + .forwardSwitchCondToPhi(ForwardSwitchCond)
> > + .convertSwitchToLookupTable(ConvertSwitch)
> > + .needCanonicalLoops(KeepLoops)
> > + .sinkCommonInsts(SinkCommon),
> > + std::move(Ftor));
> > }
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list