[PATCH] D26252: [AliasSetTracker] Make AST smarter about assume intrinsics that don't actually affect memory.

[Hal Finkel via llvm-commits]

Hi Danny,

Yes, I think this is okay. We obviously can't remove control dependencies on assume but this change should not adversely affect that guarantee. While an assume might be a function of loaded values, those loaded values have their own aliasing-based code-motion constraints and the associated value dependencies should take care of the rest. To put it another way, we don't get to make assumptions on memory contents, only on SSA values.

In short, our existing logic for handling side-effecting instructions should prevent moving the assume past possibly-throwing functions, and I can't think of any case in which this will cause a problem. Admittedly, perhaps my imagination is a bit too limited.

 -Hal

----- Original Message -----
> From: "Daniel Berlin" <dberlin at dberlin.org>
> To: reviews+D26252+public+8b533c9305de3d6e at reviews.llvm.org
> Cc: "Hal Finkel" <hfinkel at anl.gov>, "llvm-commits" <llvm-commits at lists.llvm.org>
> Sent: Monday, November 7, 2016 11:35:14 AM
> Subject: Re: [PATCH] D26252: [AliasSetTracker] Make AST smarter about assume intrinsics that don't actually affect
> memory.
> 
> 
> So, just to make sure i understand our thoughts on this:
> 
> 
> (and as a preliminary aside, when discussed at the dev meeting, it
> turns out nobody seems to be using assume anymore for it's original
> purpose. The only users anyone thought was still using it was for
> alignment and in the inliner, both of which we probably could just
> do better without the generality of assume)
> 
> 
> Before this patch, assumes effectively acted as barriers.
> 
> 
> That is, they would not be moved, but also, things would not be moved
> around them, because they were considered to alias.
> 
> 
> Is it okay to effectively move them by moving things around them?
> 
> 
> 
> To give a simple example:
> 
> 
> assume
> A
> B
> C
> D
> 
> 
> we can now move A,B,C,D up
> 
> 
> A
> B
> C
> D
> assume
> 
> 
> Or the inverse.
> 
> 
> Because without more, this patch will enable this to happen.
> (obviously, the right answer to "does it alias" is "no", but i'm
> asking more whether the world is really ready for that)
> 
> 
> On Mon, Nov 7, 2016 at 6:21 AM, Chad Rosier < mcrosier at codeaurora.org
> > wrote:
> 
> 
> This revision was automatically updated to reflect the committed
> changes.
> Closed by commit rL286108: [AliasSetTracker] Make AST smarter about
> assume intrinsics that don't actually… (authored by mcrosier).
> 
> Changed prior to commit:
> https://reviews.llvm.org/D26252?vs=76738&id=77028#toc
> 
> Repository:
> rL LLVM
> 
> https://reviews.llvm.org/D26252
> 
> Files:
> llvm/trunk/lib/Analysis/AliasSetTracker.cpp
> llvm/trunk/test/Analysis/AliasSet/intrinsics.ll
> 
> 
> Index: llvm/trunk/lib/Analysis/AliasSetTracker.cpp
> ===================================================================
> --- llvm/trunk/lib/Analysis/AliasSetTracker.cpp
> +++ llvm/trunk/lib/Analysis/AliasSetTracker.cpp
> @@ -413,6 +413,18 @@
> void AliasSetTracker::addUnknown(Instruction *Inst) {
> if (isa<DbgInfoIntrinsic>(Inst))
> return; // Ignore DbgInfo Intrinsics.
> +
> + if (auto *II = dyn_cast<IntrinsicInst>(Inst)) {
> + // These intrinsics will show up as affecting memory, but they are
> just
> + // markers.
> + switch (II->getIntrinsicID()) {
> + default:
> + break;
> + // FIXME: Add lifetime/invariant intrinsics (See: PR30807).
> + case Intrinsic::assume:
> + return;
> + }
> + }
> if (!Inst->mayReadOrWriteMemory())
> return; // doesn't alias anything
> 
> Index: llvm/trunk/test/Analysis/AliasSet/intrinsics.ll
> ===================================================================
> --- llvm/trunk/test/Analysis/AliasSet/intrinsics.ll
> +++ llvm/trunk/test/Analysis/AliasSet/intrinsics.ll
> 
> 
> @@ -0,0 +1,19 @@
> +; RUN: opt -basicaa -print-alias-sets -S -o - < %s 2>&1 | FileCheck
> %s
> +
> +; CHECK: Alias sets for function 'test1':
> +; CHECK: Alias Set Tracker: 2 alias sets for 2 pointer values.
> +; CHECK: AliasSet[0x{{[0-9a-f]+}}, 1] must alias, Mod Pointers: (i8*
> %a, 1)
> +; CHECK-NOT: 1 Unknown instruction
> +; CHECK: AliasSet[0x{{[0-9a-f]+}}, 1] must alias, Mod Pointers: (i8*
> %b, 1)
> +define void @test1(i32 %c) {
> +entry:
> + %a = alloca i8, align 1
> + %b = alloca i8, align 1
> + store i8 1, i8* %a, align 1
> + %cond1 = icmp ne i32 %c, 0
> + call void @llvm.assume(i1 %cond1)
> + store i8 1, i8* %b, align 1
> + ret void
> +}
> +
> +declare void @llvm.assume(i1)
> 
> 
> 
> 

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory