[llvm] e1eed6c - [Attributor] Generalize `getAssumedConstantInt` interface

[Florian Hahn via llvm-commits]

> On 20 Feb 2020, at 05:35, Johannes Doerfert via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> 
> Author: Johannes Doerfert
> Date: 2020-02-19T22:33:51-06:00
> New Revision: e1eed6c5b9faf89491beaa592180a1c96fe13e0e
> 
> URL: https://github.com/llvm/llvm-project/commit/e1eed6c5b9faf89491beaa592180a1c96fe13e0e
> DIFF: https://github.com/llvm/llvm-project/commit/e1eed6c5b9faf89491beaa592180a1c96fe13e0e.diff
> 
> LOG: [Attributor] Generalize `getAssumedConstantInt` interface
> 
> We are often interested in an assumed constant and sometimes it has to
> be an integer constant. Before we only looked for the latter, now we can
> ask for either.
> 
> Added: 
> 
> 
> Modified: 
>    llvm/lib/Transforms/IPO/Attributor.cpp
>    llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll
>    llvm/test/Transforms/Attributor/ArgumentPromotion/chained.ll
>    llvm/test/Transforms/Attributor/ArgumentPromotion/live_called_from_dead_2.ll
>    llvm/test/Transforms/Attributor/IPConstantProp/arg-count-mismatch.ll
>    llvm/test/Transforms/Attributor/IPConstantProp/musttail-call.ll
>    llvm/test/Transforms/Attributor/IPConstantProp/pthreads.ll
>    llvm/test/Transforms/Attributor/IPConstantProp/thread_local_acs.ll
>    llvm/test/Transforms/Attributor/align.ll
>    llvm/test/Transforms/Attributor/liveness.ll
>    llvm/test/Transforms/Attributor/value-simplify.ll
> 
> Removed: 
> 
> 
> 
> ################################################################################
> diff  --git a/llvm/lib/Transforms/IPO/Attributor.cpp b/llvm/lib/Transforms/IPO/Attributor.cpp
> index 7d6ea0fa2db4..275068d8c274 100644
> --- a/llvm/lib/Transforms/IPO/Attributor.cpp
> +++ b/llvm/lib/Transforms/IPO/Attributor.cpp
> @@ -248,9 +248,9 @@ Argument *IRPosition::getAssociatedArgument() const {
>   return nullptr;
> }
> 
> -static Optional<ConstantInt *>
> -getAssumedConstant(Attributor &A, const Value &V, const AbstractAttribute &AA,
> -                   bool &UsedAssumedInformation) {
> +static Optional<Constant *> getAssumedConstant(Attributor &A, const Value &V,
> +                                               const AbstractAttribute &AA,
> +                                               bool &UsedAssumedInformation) {
>   const auto &ValueSimplifyAA = A.getAAFor<AAValueSimplify>(
>       AA, IRPosition::value(V), /* TrackDependence */ false);
>   Optional<Value *> SimplifiedV = ValueSimplifyAA.getAssumedSimplifiedValue(A);
> @@ -264,12 +264,26 @@ getAssumedConstant(Attributor &A, const Value &V, const AbstractAttribute &AA,
>     A.recordDependence(ValueSimplifyAA, AA, DepClassTy::OPTIONAL);
>     return llvm::None;
>   }
> -  ConstantInt *CI = dyn_cast_or_null<ConstantInt>(SimplifiedV.getValue());
> +  Constant *CI = dyn_cast_or_null<Constant>(SimplifiedV.getValue());
> +  if (CI && CI->getType() != V.getType()) {
> +    // TODO: Check for a save conversion.
> +    return nullptr;
> +  }
>   if (CI)
>     A.recordDependence(ValueSimplifyAA, AA, DepClassTy::OPTIONAL);
>   return CI;
> }
> 
> +static Optional<ConstantInt *>
> +getAssumedConstantInt(Attributor &A, const Value &V,
> +                      const AbstractAttribute &AA,
> +                      bool &UsedAssumedInformation) {
> +  Optional<Constant *> C = getAssumedConstant(A, V, AA, UsedAssumedInformation);
> +  if (C.hasValue())
> +    return dyn_cast_or_null<ConstantInt>(C.getValue());
> +  return llvm::None;
> +}
> +
> /// Get pointer operand of memory accessing instruction. If \p I is
> /// not a memory accessing instruction, return nullptr. If \p AllowVolatile,
> /// is set to false and the instruction is volatile, return nullptr.
> @@ -2919,9 +2933,9 @@ struct AAIsDeadFloating : public AAIsDeadValueImpl {
>       return ChangeStatus::UNCHANGED;
> 
>     bool UsedAssumedInformation = false;
> -    Optional<ConstantInt *> CI =
> +    Optional<Constant *> C =
>         getAssumedConstant(A, V, *this, UsedAssumedInformation);
> -    if (CI.hasValue() && CI.getValue())
> +    if (C.hasValue() && C.getValue())
>       return ChangeStatus::UNCHANGED;
> 
>     UndefValue &UV = *UndefValue::get(V.getType());
> @@ -3307,8 +3321,8 @@ identifyAliveSuccessors(Attributor &A, const BranchInst &BI,
>   if (BI.getNumSuccessors() == 1) {
>     AliveSuccessors.push_back(&BI.getSuccessor(0)->front());
>   } else {
> -    Optional<ConstantInt *> CI =
> -        getAssumedConstant(A, *BI.getCondition(), AA, UsedAssumedInformation);
> +    Optional<ConstantInt *> CI = getAssumedConstantInt(
> +        A, *BI.getCondition(), AA, UsedAssumedInformation);
>     if (!CI.hasValue()) {
>       // No value yet, assume both edges are dead.
>     } else if (CI.getValue()) {
> @@ -3330,7 +3344,7 @@ identifyAliveSuccessors(Attributor &A, const SwitchInst &SI,
>                         SmallVectorImpl<const Instruction *> &AliveSuccessors) {
>   bool UsedAssumedInformation = false;
>   Optional<ConstantInt *> CI =
> -      getAssumedConstant(A, *SI.getCondition(), AA, UsedAssumedInformation);
> +      getAssumedConstantInt(A, *SI.getCondition(), AA, UsedAssumedInformation);
>   if (!CI.hasValue()) {
>     // No value yet, assume all edges are dead.
>   } else if (CI.getValue()) {
> @@ -7220,11 +7234,11 @@ bool Attributor::checkForAllUses(
>   // instead use the `follow` callback argument to look at transitive users,
>   // however, that should be clear from the presence of the argument.
>   bool UsedAssumedInformation = false;
> -  Optional<ConstantInt *> CI =
> +  Optional<Constant *> C =
>       getAssumedConstant(*this, V, QueryingAA, UsedAssumedInformation);
> -  if (CI.hasValue() && CI.getValue()) {
> +  if (C.hasValue() && C.getValue()) {
>     LLVM_DEBUG(dbgs() << "[Attributor] Value is simplified, uses skipped: " << V
> -                      << " -> " << *CI.getValue() << "\n");
> +                      << " -> " << *C.getValue() << "\n");
>     return true;
>   }
> 
> 
> diff  --git a/llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll b/llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll
> index 717181afb324..bea9c3381438 100644
> --- a/llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll
> +++ b/llvm/test/Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll
> @@ -5,8 +5,7 @@
> ; This test tries to convince CHECK about promoting the load from %A + 2,
> ; because there is a load of %A in the entry block
> define internal i32 @callee(i1 %C, i32* %A) {
> -; CHECK-LABEL: define {{[^@]+}}@callee
> -; CHECK-SAME: (i32* noalias nocapture nofree nonnull readonly align 536870912 dereferenceable(4) [[A:%.*]])
> +; CHECK-LABEL: define {{[^@]+}}@callee()
> ; CHECK-NEXT:  entry:

Is there a reason the CHECK-SAME line was dropped? Are there no attributes after the change?

In general, it also seems like there are a lot smallish Attributor patches landing without review. I think at least for some of them it might be good to submit them for review, even if it only helps with increasing the number of people who are a bit more familiar with it.

Cheers,
Florian