[llvm] e1eed6c - [Attributor] Generalize `getAssumedConstantInt` interface
Johannes Doerfert via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 20 08:03:22 PST 2020
On 02/20, Florian Hahn wrote:
>
>
> > 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?
The update test script did the change. It is written to not use a
separate check line if there are no arguments. This is because the
CHECK-SAME lines is actually an artifact of CHECK-LABEL not being able
to contain regexps. The optimization here dropped the unused argument
which was the only one present, thus removing the need for the
CHECK-SAME line. If that behavior is to be changed we need to do it in
the update scrip.
> 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.
You are right, I land smallish Attributor changes, and especially fixes,
w/o pre-review. I already try to judge what features/changes should be
discussed first. I don't object to post-review discussions or an
adjustment of my strategy. If you think I should be more conservative
and put more of them on phab first, I can do that. If you have felt that
way on past patches, feel free to point them out to me in private or on
the list so I get a better feeling for the request, assuming you don't
want me to put all my Attributor patches on phab first. That said, if
you know people that actually want to review such patches, please get
them involved. The lack of reviewers is certainly one of the biggest
challenges we all face.
TBH, even with your comment I still don't think this patch needed a
review though. It removed a design flaw that went in unnoticed and did
not change the intent of the code. I mean, it was never the intention to
only deal with ConstantInt but not other Constant values. I hope that
makes sense.
Cheers,
Johannes
P.S. There is https://reviews.llvm.org/D74888 :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200220/9861ee03/attachment.sig>
More information about the llvm-commits
mailing list