[llvm] 931b606 - [BasicAA] Handle assumes with operand bundles
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 24 14:56:39 PDT 2021
On 3/24/21 2:12 PM, Nikita Popov wrote:
> On Wed, Mar 24, 2021 at 10:06 PM Philip Reames
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>
>
> On 3/24/21 10:04 AM, Nikita Popov wrote:
>> On Wed, Mar 24, 2021 at 1:10 AM Philip Reames
>> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>>
>>
>> On 3/23/21 2:35 PM, Nikita Popov wrote:
>>> On Tue, Mar 23, 2021 at 10:21 PM Philip Reames
>>> <listmail at philipreames.com
>>> <mailto:listmail at philipreames.com>> wrote:
>>>
>>> This doesn't seem like the right fix. We conservatively
>>> report operand
>>> bundles allowing reads for *unknown* bundle types. The
>>> assume bundle
>>> type should have well defined semantics and could be
>>> explicitly whitelisted.
>>>
>>> Philip
>>>
>>>
>>> I believe the assume operand bundles are somewhat special
>>> and distinct from other bundles, in that they only apply to
>>> assumes, and in that they are not explicitly enumerated. An
>>> operand bundle on an assume simply corresponds to the
>>> attribute of the same name.
>> Right, that's my point. See isFnAttrDisallowedByOpBundle and
>> callees on CallBase. You should be able to simply add
>> explicit handling for assume bundle there and everything
>> "should work".
>>
>>
>> Okay, I think I just misunderstood what you were suggesting
>> initially. Does
>> https://gist.github.com/nikic/0f9733122887065d656d0301e47504a1
>> <https://gist.github.com/nikic/0f9733122887065d656d0301e47504a1>
>> match what you have in mind here?
> Yep. I think you've got a slight bug there in that you do want
> assumes to access inaccessiblememory, but the general code
> structure is exactly what I meant.
>
>
> Not sure I spot the bug: isFnAttrDisallowedByOpBundle() calls
> hasReadingOperandBundles(), which will now return false for assumes.
> This means that hasFnAttrOnCalledFunction() gets called and returns
> true for the InaccessibleMemOnly attribute.
You are correct. I'd missed the last step (that the function decl was
correct.)
>
> Regards,
> Nikita
>
>>> On 3/23/21 1:21 PM, Nikita Popov via llvm-commits wrote:
>>> > Author: Nikita Popov
>>> > Date: 2021-03-23T21:21:19+01:00
>>> > New Revision: 931b6066acc597e680b8df25521a2f83b40dfaae
>>> >
>>> > URL:
>>> https://github.com/llvm/llvm-project/commit/931b6066acc597e680b8df25521a2f83b40dfaae
>>> <https://github.com/llvm/llvm-project/commit/931b6066acc597e680b8df25521a2f83b40dfaae>
>>> > DIFF:
>>> https://github.com/llvm/llvm-project/commit/931b6066acc597e680b8df25521a2f83b40dfaae.diff
>>> <https://github.com/llvm/llvm-project/commit/931b6066acc597e680b8df25521a2f83b40dfaae.diff>
>>> >
>>> > LOG: [BasicAA] Handle assumes with operand bundles
>>> >
>>> > This fixes a regression reported on D99022: If a call
>>> has operand
>>> > bundles, then the inaccessiblememonly attribute on the
>>> function
>>> > will be ignored, as operand bundles can affect modref
>>> behavior in
>>> > the general case. However, for assume operand bundles
>>> in particular
>>> > this is not the case.
>>> >
>>> > Adjust getModRefBehavior() to always report
>>> inaccessiblememonly
>>> > for assumes, regardless of presence of operand bundles.
>>> >
>>> > Added:
>>> >
>>> >
>>> > Modified:
>>> > llvm/lib/Analysis/BasicAliasAnalysis.cpp
>>> > llvm/test/Analysis/BasicAA/assume.ll
>>> >
>>> > Removed:
>>> >
>>> >
>>> >
>>> >
>>> ################################################################################
>>> > diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
>>> b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
>>> > index 86362f770e2d4..65117d82a81ce 100644
>>> > --- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
>>> > +++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
>>> > @@ -672,12 +672,22 @@ bool
>>> BasicAAResult::pointsToConstantMemory(const
>>> MemoryLocation &Loc,
>>> > return Worklist.empty();
>>> > }
>>> >
>>> > +static bool isIntrinsicCall(const CallBase *Call,
>>> Intrinsic::ID IID) {
>>> > + const IntrinsicInst *II =
>>> dyn_cast<IntrinsicInst>(Call);
>>> > + return II && II->getIntrinsicID() == IID;
>>> > +}
>>> > +
>>> > /// Returns the behavior when calling the given call
>>> site.
>>> > FunctionModRefBehavior
>>> BasicAAResult::getModRefBehavior(const CallBase *Call) {
>>> > if (Call->doesNotAccessMemory())
>>> > // Can't do better than this.
>>> > return FMRB_DoesNotAccessMemory;
>>> >
>>> > + // The assume intrinsic can have operand bundles,
>>> but still only accesses
>>> > + // inaccessible memory in that case (to maintain
>>> control dependencies).
>>> > + if (isIntrinsicCall(Call, Intrinsic::assume))
>>> > + return FMRB_OnlyAccessesInaccessibleMem;
>>> > +
>>> > FunctionModRefBehavior Min =
>>> FMRB_UnknownModRefBehavior;
>>> >
>>> > // If the callsite knows it only reads memory,
>>> don't return worse
>>> > @@ -771,11 +781,6 @@ ModRefInfo
>>> BasicAAResult::getArgModRefInfo(const CallBase *Call,
>>> > return AAResultBase::getArgModRefInfo(Call, ArgIdx);
>>> > }
>>> >
>>> > -static bool isIntrinsicCall(const CallBase *Call,
>>> Intrinsic::ID IID) {
>>> > - const IntrinsicInst *II =
>>> dyn_cast<IntrinsicInst>(Call);
>>> > - return II && II->getIntrinsicID() == IID;
>>> > -}
>>> > -
>>> > #ifndef NDEBUG
>>> > static const Function *getParent(const Value *V) {
>>> > if (const Instruction *inst =
>>> dyn_cast<Instruction>(V)) {
>>> >
>>> > diff --git a/llvm/test/Analysis/BasicAA/assume.ll
>>> b/llvm/test/Analysis/BasicAA/assume.ll
>>> > index 4b268836cc49e..bc6be3ef0157e 100644
>>> > --- a/llvm/test/Analysis/BasicAA/assume.ll
>>> > +++ b/llvm/test/Analysis/BasicAA/assume.ll
>>> > @@ -29,12 +29,12 @@ define void @test2(i8* %P, i8* %Q)
>>> nounwind ssp {
>>> > ; CHECK-LABEL: Function: test2:
>>> >
>>> > ; CHECK: MayAlias: i8* %P, i8* %Q
>>> > -; CHECK: Both ModRef: Ptr: i8* %P <-> tail call
>>> void @llvm.assume(i1 true) [ "nonnull"(i8* %P) ]
>>> > -; CHECK: Both ModRef: Ptr: i8* %Q <-> tail call
>>> void @llvm.assume(i1 true) [ "nonnull"(i8* %P) ]
>>> > +; CHECK: NoModRef: Ptr: i8* %P <-> tail call void
>>> @llvm.assume(i1 true) [ "nonnull"(i8* %P) ]
>>> > +; CHECK: NoModRef: Ptr: i8* %Q <-> tail call void
>>> @llvm.assume(i1 true) [ "nonnull"(i8* %P) ]
>>> > ; CHECK: Both ModRef: Ptr: i8* %P <-> tail call
>>> void @llvm.memcpy.p0i8.p0i8.i64(i8* %P, i8* %Q, i64 12,
>>> i1 false)
>>> > ; CHECK: Both ModRef: Ptr: i8* %Q <-> tail call
>>> void @llvm.memcpy.p0i8.p0i8.i64(i8* %P, i8* %Q, i64 12,
>>> i1 false)
>>> > -; CHECK: Both ModRef: tail call void
>>> @llvm.assume(i1 true) [ "nonnull"(i8* %P) ] <-> tail
>>> call void @llvm.memcpy.p0i8.p0i8.i64(i8* %P, i8* %Q, i64
>>> 12, i1 false)
>>> > -; CHECK: Both ModRef: tail call void
>>> @llvm.memcpy.p0i8.p0i8.i64(i8* %P, i8* %Q, i64 12, i1
>>> false) <-> tail call void @llvm.assume(i1 true) [
>>> "nonnull"(i8* %P) ]
>>> > +; CHECK: NoModRef: tail call void @llvm.assume(i1
>>> true) [ "nonnull"(i8* %P) ] <-> tail call void
>>> @llvm.memcpy.p0i8.p0i8.i64(i8* %P, i8* %Q, i64 12, i1 false)
>>> > +; CHECK: NoModRef: tail call void
>>> @llvm.memcpy.p0i8.p0i8.i64(i8* %P, i8* %Q, i64 12, i1
>>> false) <-> tail call void @llvm.assume(i1 true) [
>>> "nonnull"(i8* %P) ]
>>> > }
>>> >
>>> > attributes #0 = { nounwind }
>>> >
>>> >
>>> >
>>> > _______________________________________________
>>> > llvm-commits mailing list
>>> > llvm-commits at lists.llvm.org
>>> <mailto:llvm-commits at lists.llvm.org>
>>> >
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>> <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/20210324/b2ea1870/attachment.html>
More information about the llvm-commits
mailing list