<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 24, 2021 at 10:06 PM Philip Reames <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p><br>
</p>
<div>On 3/24/21 10:04 AM, Nikita Popov
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, Mar 24, 2021 at 1:10
AM Philip Reames <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p><br>
</p>
<div>On 3/23/21 2:35 PM, Nikita Popov wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Tue, Mar 23,
2021 at 10:21 PM Philip Reames <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This doesn't
seem like the right fix. We conservatively report
operand <br>
bundles allowing reads for *unknown* bundle
types. The assume bundle <br>
type should have well defined semantics and could
be explicitly whitelisted.<br>
<br>
Philip<br>
</blockquote>
<div><br>
</div>
<div>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.<br>
</div>
</div>
</div>
</blockquote>
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". <br>
</div>
</blockquote>
<div><br>
</div>
<div>Okay, I think I just misunderstood what you were
suggesting initially. Does <a href="https://gist.github.com/nikic/0f9733122887065d656d0301e47504a1" target="_blank">https://gist.github.com/nikic/0f9733122887065d656d0301e47504a1</a>
match what you have in mind here?</div>
</div>
</div>
</blockquote>
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.<br></div></blockquote><div><br></div>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.</div><div class="gmail_quote"><br></div><div class="gmail_quote">Regards,<br></div><div class="gmail_quote">Nikita</div><div class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><blockquote type="cite"><div dir="ltr"><div class="gmail_quote"><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> On 3/23/21
1:21 PM, Nikita Popov via llvm-commits wrote:<br>
> Author: Nikita Popov<br>
> Date: 2021-03-23T21:21:19+01:00<br>
> New Revision:
931b6066acc597e680b8df25521a2f83b40dfaae<br>
><br>
> URL: <a href="https://github.com/llvm/llvm-project/commit/931b6066acc597e680b8df25521a2f83b40dfaae" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/931b6066acc597e680b8df25521a2f83b40dfaae</a><br>
> DIFF: <a href="https://github.com/llvm/llvm-project/commit/931b6066acc597e680b8df25521a2f83b40dfaae.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/931b6066acc597e680b8df25521a2f83b40dfaae.diff</a><br>
><br>
> LOG: [BasicAA] Handle assumes with operand
bundles<br>
><br>
> This fixes a regression reported on D99022:
If a call has operand<br>
> bundles, then the inaccessiblememonly
attribute on the function<br>
> will be ignored, as operand bundles can
affect modref behavior in<br>
> the general case. However, for assume operand
bundles in particular<br>
> this is not the case.<br>
><br>
> Adjust getModRefBehavior() to always report
inaccessiblememonly<br>
> for assumes, regardless of presence of
operand bundles.<br>
><br>
> Added:<br>
> <br>
><br>
> Modified:<br>
> llvm/lib/Analysis/BasicAliasAnalysis.cpp<br>
> llvm/test/Analysis/BasicAA/assume.ll<br>
><br>
> Removed:<br>
> <br>
><br>
><br>
>
################################################################################<br>
> diff --git
a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
b/llvm/lib/Analysis/BasicAliasAnalysis.cpp<br>
> index 86362f770e2d4..65117d82a81ce 100644<br>
> ---
a/llvm/lib/Analysis/BasicAliasAnalysis.cpp<br>
> +++
b/llvm/lib/Analysis/BasicAliasAnalysis.cpp<br>
> @@ -672,12 +672,22 @@ bool
BasicAAResult::pointsToConstantMemory(const
MemoryLocation &Loc,<br>
> return Worklist.empty();<br>
> }<br>
> <br>
> +static bool isIntrinsicCall(const CallBase
*Call, Intrinsic::ID IID) {<br>
> + const IntrinsicInst *II =
dyn_cast<IntrinsicInst>(Call);<br>
> + return II &&
II->getIntrinsicID() == IID;<br>
> +}<br>
> +<br>
> /// Returns the behavior when calling the
given call site.<br>
> FunctionModRefBehavior
BasicAAResult::getModRefBehavior(const CallBase
*Call) {<br>
> if (Call->doesNotAccessMemory())<br>
> // Can't do better than this.<br>
> return FMRB_DoesNotAccessMemory;<br>
> <br>
> + // The assume intrinsic can have operand
bundles, but still only accesses<br>
> + // inaccessible memory in that case (to
maintain control dependencies).<br>
> + if (isIntrinsicCall(Call,
Intrinsic::assume))<br>
> + return FMRB_OnlyAccessesInaccessibleMem;<br>
> +<br>
> FunctionModRefBehavior Min =
FMRB_UnknownModRefBehavior;<br>
> <br>
> // If the callsite knows it only reads
memory, don't return worse<br>
> @@ -771,11 +781,6 @@ ModRefInfo
BasicAAResult::getArgModRefInfo(const CallBase
*Call,<br>
> return
AAResultBase::getArgModRefInfo(Call, ArgIdx);<br>
> }<br>
> <br>
> -static bool isIntrinsicCall(const CallBase
*Call, Intrinsic::ID IID) {<br>
> - const IntrinsicInst *II =
dyn_cast<IntrinsicInst>(Call);<br>
> - return II &&
II->getIntrinsicID() == IID;<br>
> -}<br>
> -<br>
> #ifndef NDEBUG<br>
> static const Function *getParent(const
Value *V) {<br>
> if (const Instruction *inst =
dyn_cast<Instruction>(V)) {<br>
><br>
> diff --git
a/llvm/test/Analysis/BasicAA/assume.ll
b/llvm/test/Analysis/BasicAA/assume.ll<br>
> index 4b268836cc49e..bc6be3ef0157e 100644<br>
> --- a/llvm/test/Analysis/BasicAA/assume.ll<br>
> +++ b/llvm/test/Analysis/BasicAA/assume.ll<br>
> @@ -29,12 +29,12 @@ define void @test2(i8*
%P, i8* %Q) nounwind ssp {<br>
> ; CHECK-LABEL: Function: test2:<br>
> <br>
> ; CHECK: MayAlias: i8* %P, i8* %Q<br>
> -; CHECK: Both ModRef: Ptr: i8* %P
<-> tail call void @llvm.assume(i1 true) [
"nonnull"(i8* %P) ]<br>
> -; CHECK: Both ModRef: Ptr: i8* %Q
<-> tail call void @llvm.assume(i1 true) [
"nonnull"(i8* %P) ]<br>
> +; CHECK: NoModRef: Ptr: i8* %P
<-> tail call void @llvm.assume(i1 true) [
"nonnull"(i8* %P) ]<br>
> +; CHECK: NoModRef: Ptr: i8* %Q
<-> tail call void @llvm.assume(i1 true) [
"nonnull"(i8* %P) ]<br>
> ; CHECK: Both ModRef: Ptr: i8* %P
<-> tail call void
@llvm.memcpy.p0i8.p0i8.i64(i8* %P, i8* %Q, i64 12,
i1 false)<br>
> ; CHECK: Both ModRef: Ptr: i8* %Q
<-> tail call void
@llvm.memcpy.p0i8.p0i8.i64(i8* %P, i8* %Q, i64 12,
i1 false)<br>
> -; 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)<br>
> -; 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) ]<br>
> +; 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)<br>
> +; 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) ]<br>
> }<br>
> <br>
> attributes #0 = { nounwind }<br>
><br>
><br>
> <br>
>
_______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
</div>
</blockquote></div></div>