[llvm] e1eed6c - [Attributor] Generalize `getAssumedConstantInt` interface
Florian Hahn via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 20 13:50:08 PST 2020
> On 20 Feb 2020, at 17:03, Johannes Doerfert <johannesdoerfert at gmail.com> wrote:
>
> 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.
>
Right, thanks!
>
>> 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.
It was nothing specific about this patch, just a general thought/suggestion, mostly because I thought it might help getting a few more people familiar with it. I think your judgement makes sense though and there’s no clear line anyways :)
Cheers,
Florian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200220/4e8ead68/attachment.html>
More information about the llvm-commits
mailing list