[llvm] c57689b - [Attributor][NFC] Copy llvm::function_ref, don't use references
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 23 11:17:46 PDT 2020
On Mon, Mar 23, 2020 at 8:47 AM Johannes Doerfert via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
> Author: Johannes Doerfert
> Date: 2020-03-23T10:45:24-05:00
> New Revision: c57689bef261e18c4c5de70a2b9a1864462a0006
>
> URL:
> https://github.com/llvm/llvm-project/commit/c57689bef261e18c4c5de70a2b9a1864462a0006
> DIFF:
> https://github.com/llvm/llvm-project/commit/c57689bef261e18c4c5de70a2b9a1864462a0006.diff
>
> LOG: [Attributor][NFC] Copy llvm::function_ref, don't use references
>
> On IRC this was called a "code smell" so we get rid of it.
>
To expound upon this a bit more/provide a more concrete rationalization to
that phrase: llvm::function_ref is trivially copyable and the size of two
pointers, so passing it by reference would be like passing a pair of
pointers by reference which usually wouldn't be done & vould raise
questions about whether the referenceness was special in this case (is the
original object going to be indirectly modified in some way during the
execution of this function in such a way that this function needs to
observe that change? Otherwise you'd pass by value)
Thanks for the cleanup! Sorry we still don't have a great answer on what
went wrong on that other patch where this helped (well, I narrowed it down
after commit/revert to just capturing by reference in the lambdas, without
the change to the outer function signature, but that's still not conclusive)
- Dave
>
> Added:
>
>
> Modified:
> llvm/include/llvm/Transforms/IPO/Attributor.h
> llvm/lib/Transforms/IPO/Attributor.cpp
>
> Removed:
>
>
>
>
> ################################################################################
> diff --git a/llvm/include/llvm/Transforms/IPO/Attributor.h
> b/llvm/include/llvm/Transforms/IPO/Attributor.h
> index 41d3c2f9ec6d..1df8ce9f17ff 100644
> --- a/llvm/include/llvm/Transforms/IPO/Attributor.h
> +++ b/llvm/include/llvm/Transforms/IPO/Attributor.h
> @@ -894,7 +894,7 @@ struct Attributor {
> ///
> /// This method will evaluate \p Pred on all (transitive) uses of the
> /// associated value and return true if \p Pred holds every time.
> - bool checkForAllUses(const function_ref<bool(const Use &, bool &)>
> &Pred,
> + bool checkForAllUses(function_ref<bool(const Use &, bool &)> Pred,
> const AbstractAttribute &QueryingAA, const Value
> &V,
> DepClassTy LivenessDepClass =
> DepClassTy::OPTIONAL);
>
> @@ -1006,7 +1006,7 @@ struct Attributor {
> /// all call sites are known, hence the function has internal linkage.
> /// If true is returned, \p AllCallSitesKnown is set if all possible
> call
> /// sites of the function have been visited.
> - bool checkForAllCallSites(const function_ref<bool(AbstractCallSite)>
> &Pred,
> + bool checkForAllCallSites(function_ref<bool(AbstractCallSite)> Pred,
> const AbstractAttribute &QueryingAA,
> bool RequireAllCallSites, bool
> &AllCallSitesKnown);
>
> @@ -1017,22 +1017,21 @@ struct Attributor {
> /// matched with their respective return instructions. Returns true if
> \p Pred
> /// holds on all of them.
> bool checkForAllReturnedValuesAndReturnInsts(
> - const function_ref<bool(Value &, const SmallSetVector<ReturnInst *,
> 4> &)>
> - &Pred,
> + function_ref<bool(Value &, const SmallSetVector<ReturnInst *, 4>
> &)> Pred,
> const AbstractAttribute &QueryingAA);
>
> /// Check \p Pred on all values potentially returned by the function
> /// associated with \p QueryingAA.
> ///
> /// This is the context insensitive version of the method above.
> - bool checkForAllReturnedValues(const function_ref<bool(Value &)> &Pred,
> + bool checkForAllReturnedValues(function_ref<bool(Value &)> Pred,
> const AbstractAttribute &QueryingAA);
>
> /// Check \p Pred on all instructions with an opcode present in \p
> Opcodes.
> ///
> /// This method will evaluate \p Pred on all instructions with an opcode
> /// present in \p Opcode and return true if \p Pred holds on all of
> them.
> - bool checkForAllInstructions(const function_ref<bool(Instruction &)>
> &Pred,
> + bool checkForAllInstructions(function_ref<bool(Instruction &)> Pred,
> const AbstractAttribute &QueryingAA,
> const ArrayRef<unsigned> &Opcodes,
> bool CheckBBLivenessOnly = false);
> @@ -1040,9 +1039,8 @@ struct Attributor {
> /// Check \p Pred on all call-like instructions (=CallBased derived).
> ///
> /// See checkForAllCallLikeInstructions(...) for more information.
> - bool
> - checkForAllCallLikeInstructions(const function_ref<bool(Instruction &)>
> &Pred,
> - const AbstractAttribute &QueryingAA) {
> + bool checkForAllCallLikeInstructions(function_ref<bool(Instruction &)>
> Pred,
> + const AbstractAttribute
> &QueryingAA) {
> return checkForAllInstructions(Pred, QueryingAA,
> {(unsigned)Instruction::Invoke,
> (unsigned)Instruction::CallBr,
> @@ -1054,9 +1052,8 @@ struct Attributor {
> /// This method will evaluate \p Pred on all instructions that read or
> write
> /// to memory present in the information cache and return true if \p
> Pred
> /// holds on all of them.
> - bool checkForAllReadWriteInstructions(
> - const llvm::function_ref<bool(Instruction &)> &Pred,
> - AbstractAttribute &QueryingAA);
> + bool checkForAllReadWriteInstructions(function_ref<bool(Instruction &)>
> Pred,
> + AbstractAttribute &QueryingAA);
>
> /// Return the data layout associated with the anchor scope.
> const DataLayout &getDataLayout() const { return InfoCache.DL; }
> @@ -1069,7 +1066,7 @@ struct Attributor {
> /// all call sites are known, hence the function has internal linkage.
> /// If true is returned, \p AllCallSitesKnown is set if all possible
> call
> /// sites of the function have been visited.
> - bool checkForAllCallSites(const function_ref<bool(AbstractCallSite)>
> &Pred,
> + bool checkForAllCallSites(function_ref<bool(AbstractCallSite)> Pred,
> const Function &Fn, bool RequireAllCallSites,
> const AbstractAttribute *QueryingAA,
> bool &AllCallSitesKnown);
> @@ -1918,8 +1915,8 @@ struct AAReturnedValues
> /// Note: Unlike the Attributor::checkForAllReturnedValuesAndReturnInsts
> /// method, this one will not filter dead return instructions.
> virtual bool checkForAllReturnedValuesAndReturnInsts(
> - const function_ref<bool(Value &, const SmallSetVector<ReturnInst *,
> 4> &)>
> - &Pred) const = 0;
> + function_ref<bool(Value &, const SmallSetVector<ReturnInst *, 4>
> &)> Pred)
> + const = 0;
>
> using iterator =
> MapVector<Value *, SmallSetVector<ReturnInst *, 4>>::iterator;
> @@ -2723,8 +2720,9 @@ struct AAMemoryLocation
> /// underlying accessed memory pointer) and it will return true if \p
> Pred
> /// holds every time.
> virtual bool checkForAllAccessesToMemoryKind(
> - const function_ref<bool(const Instruction *, const Value *,
> AccessKind,
> - MemoryLocationsKind)> &Pred,
> + function_ref<bool(const Instruction *, const Value *, AccessKind,
> + MemoryLocationsKind)>
> + Pred,
> MemoryLocationsKind MLK) const = 0;
>
> /// Create an abstract attribute view for the position \p IRP.
>
> diff --git a/llvm/lib/Transforms/IPO/Attributor.cpp
> b/llvm/lib/Transforms/IPO/Attributor.cpp
> index 594e7146d2b9..2111f9ffac42 100644
> --- a/llvm/lib/Transforms/IPO/Attributor.cpp
> +++ b/llvm/lib/Transforms/IPO/Attributor.cpp
> @@ -398,8 +398,8 @@ static Value *constructPointer(Type *ResTy, Value
> *Ptr, int64_t Offset,
> template <typename AAType, typename StateTy>
> static bool genericValueTraversal(
> Attributor &A, IRPosition IRP, const AAType &QueryingAA, StateTy
> &State,
> - const function_ref<bool(Value &, StateTy &, bool)> &VisitValueCB,
> - int MaxValues = 8, const function_ref<Value *(Value *)> StripCB =
> nullptr) {
> + function_ref<bool(Value &, StateTy &, bool)> VisitValueCB,
> + int MaxValues = 8, function_ref<Value *(Value *)> StripCB = nullptr) {
>
> const AAIsDead *LivenessAA = nullptr;
> if (IRP.getAnchorScope())
> @@ -1272,8 +1272,8 @@ class AAReturnedValuesImpl : public
> AAReturnedValues, public AbstractState {
>
> /// See AbstractState::checkForAllReturnedValues(...).
> bool checkForAllReturnedValuesAndReturnInsts(
> - const function_ref<bool(Value &, const SmallSetVector<ReturnInst *,
> 4> &)>
> - &Pred) const override;
> + function_ref<bool(Value &, const SmallSetVector<ReturnInst *, 4>
> &)> Pred)
> + const override;
>
> /// Pretty print the attribute similar to the IR representation.
> const std::string getAsStr() const override;
> @@ -1399,8 +1399,8 @@
> AAReturnedValuesImpl::getAssumedUniqueReturnValue(Attributor &A) const {
> }
>
> bool AAReturnedValuesImpl::checkForAllReturnedValuesAndReturnInsts(
> - const function_ref<bool(Value &, const SmallSetVector<ReturnInst *,
> 4> &)>
> - &Pred) const {
> + function_ref<bool(Value &, const SmallSetVector<ReturnInst *, 4> &)>
> Pred)
> + const {
> if (!isValidState())
> return false;
>
> @@ -6384,8 +6384,9 @@ struct AAMemoryLocationImpl : public
> AAMemoryLocation {
>
> /// See AAMemoryLocation::checkForAllAccessesToMemoryKind(...).
> bool checkForAllAccessesToMemoryKind(
> - const function_ref<bool(const Instruction *, const Value *,
> AccessKind,
> - MemoryLocationsKind)> &Pred,
> + function_ref<bool(const Instruction *, const Value *, AccessKind,
> + MemoryLocationsKind)>
> + Pred,
> MemoryLocationsKind RequestedMLK) const override {
> if (!isValidState())
> return false;
> @@ -7366,10 +7367,9 @@ bool Attributor::isAssumedDead(const IRPosition
> &IRP,
> return false;
> }
>
> -bool Attributor::checkForAllUses(
> - const function_ref<bool(const Use &, bool &)> &Pred,
> - const AbstractAttribute &QueryingAA, const Value &V,
> - DepClassTy LivenessDepClass) {
> +bool Attributor::checkForAllUses(function_ref<bool(const Use &, bool &)>
> Pred,
> + const AbstractAttribute &QueryingAA,
> + const Value &V, DepClassTy
> LivenessDepClass) {
>
> // Check the trivial case first as it catches void values.
> if (V.use_empty())
> @@ -7428,10 +7428,10 @@ bool Attributor::checkForAllUses(
> return true;
> }
>
> -bool Attributor::checkForAllCallSites(
> - const function_ref<bool(AbstractCallSite)> &Pred,
> - const AbstractAttribute &QueryingAA, bool RequireAllCallSites,
> - bool &AllCallSitesKnown) {
> +bool
> Attributor::checkForAllCallSites(function_ref<bool(AbstractCallSite)> Pred,
> + const AbstractAttribute &QueryingAA,
> + bool RequireAllCallSites,
> + bool &AllCallSitesKnown) {
> // We can try to determine information from
> // the call sites. However, this is only possible all call sites are
> known,
> // hence the function has internal linkage.
> @@ -7448,10 +7448,11 @@ bool Attributor::checkForAllCallSites(
> &QueryingAA, AllCallSitesKnown);
> }
>
> -bool Attributor::checkForAllCallSites(
> - const function_ref<bool(AbstractCallSite)> &Pred, const Function &Fn,
> - bool RequireAllCallSites, const AbstractAttribute *QueryingAA,
> - bool &AllCallSitesKnown) {
> +bool
> Attributor::checkForAllCallSites(function_ref<bool(AbstractCallSite)> Pred,
> + const Function &Fn,
> + bool RequireAllCallSites,
> + const AbstractAttribute *QueryingAA,
> + bool &AllCallSitesKnown) {
> if (RequireAllCallSites && !Fn.hasLocalLinkage()) {
> LLVM_DEBUG(
> dbgs()
> @@ -7533,8 +7534,7 @@ bool Attributor::checkForAllCallSites(
> }
>
> bool Attributor::checkForAllReturnedValuesAndReturnInsts(
> - const function_ref<bool(Value &, const SmallSetVector<ReturnInst *,
> 4> &)>
> - &Pred,
> + function_ref<bool(Value &, const SmallSetVector<ReturnInst *, 4> &)>
> Pred,
> const AbstractAttribute &QueryingAA) {
>
> const IRPosition &IRP = QueryingAA.getIRPosition();
> @@ -7556,8 +7556,7 @@ bool
> Attributor::checkForAllReturnedValuesAndReturnInsts(
> }
>
> bool Attributor::checkForAllReturnedValues(
> - const function_ref<bool(Value &)> &Pred,
> - const AbstractAttribute &QueryingAA) {
> + function_ref<bool(Value &)> Pred, const AbstractAttribute
> &QueryingAA) {
>
> const IRPosition &IRP = QueryingAA.getIRPosition();
> const Function *AssociatedFunction = IRP.getAssociatedFunction();
> @@ -7578,9 +7577,9 @@ bool Attributor::checkForAllReturnedValues(
>
> static bool checkForAllInstructionsImpl(
> Attributor *A, InformationCache::OpcodeInstMapTy &OpcodeInstMap,
> - const function_ref<bool(Instruction &)> &Pred,
> - const AbstractAttribute *QueryingAA, const AAIsDead *LivenessAA,
> - const ArrayRef<unsigned> &Opcodes, bool CheckBBLivenessOnly = false) {
> + function_ref<bool(Instruction &)> Pred, const AbstractAttribute
> *QueryingAA,
> + const AAIsDead *LivenessAA, const ArrayRef<unsigned> &Opcodes,
> + bool CheckBBLivenessOnly = false) {
> for (unsigned Opcode : Opcodes) {
> for (Instruction *I : OpcodeInstMap[Opcode]) {
> // Skip dead instructions.
> @@ -7595,10 +7594,10 @@ static bool checkForAllInstructionsImpl(
> return true;
> }
>
> -bool Attributor::checkForAllInstructions(
> - const llvm::function_ref<bool(Instruction &)> &Pred,
> - const AbstractAttribute &QueryingAA, const ArrayRef<unsigned>
> &Opcodes,
> - bool CheckBBLivenessOnly) {
> +bool Attributor::checkForAllInstructions(function_ref<bool(Instruction
> &)> Pred,
> + const AbstractAttribute
> &QueryingAA,
> + const ArrayRef<unsigned>
> &Opcodes,
> + bool CheckBBLivenessOnly) {
>
> const IRPosition &IRP = QueryingAA.getIRPosition();
> // Since we need to provide instructions we have to have an exact
> definition.
> @@ -7621,8 +7620,7 @@ bool Attributor::checkForAllInstructions(
> }
>
> bool Attributor::checkForAllReadWriteInstructions(
> - const llvm::function_ref<bool(Instruction &)> &Pred,
> - AbstractAttribute &QueryingAA) {
> + function_ref<bool(Instruction &)> Pred, AbstractAttribute
> &QueryingAA) {
>
> const Function *AssociatedFunction =
> QueryingAA.getIRPosition().getAssociatedFunction();
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200323/679f0fd2/attachment.html>
More information about the llvm-commits
mailing list