[PATCH] Sink: Don't sink allocas

[Hal Finkel]

----- Original Message -----
> From: "Tom Stellard" <tom at stellard.net>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu, "Tom Stellard" <thomas.stellard at amd.com>, "Matt Arsenault" <Matthew.Arsenault at amd.com>
> Sent: Wednesday, January 22, 2014 12:35:54 PM
> Subject: Re: [PATCH] Sink: Don't sink allocas
> 
> Hi,
> 
> Here is an updated patch that only prevents the sinking
> of static allocas from the entry block.

LGTM. Thanks!

 -Hal

> 
> -Tom
> 
> On Mon, Jan 20, 2014 at 03:09:46PM -0600, Hal Finkel wrote:
> > ----- Original Message -----
> > > From: "Tom Stellard" <tom at stellard.net>
> > > To: "Hal Finkel" <hfinkel at anl.gov>
> > > Cc: llvm-commits at cs.uiuc.edu, "Tom Stellard"
> > > <thomas.stellard at amd.com>, "Matt Arsenault"
> > > <Matthew.Arsenault at amd.com>
> > > Sent: Monday, January 20, 2014 3:03:33 PM
> > > Subject: Re: [PATCH] Sink: Don't sink allocas
> > > 
> > > On Mon, Jan 20, 2014 at 02:58:26PM -0600, Hal Finkel wrote:
> > > > Tom,
> > > > 
> > > > Do we never want to sink allocas, or just never want to sink
> > > > non-dynamic ones out of the entry block? I think it is the
> > > > latter,
> > > > and that we still want to sink dynamic allocas. Am I missing
> > > > something?
> > > 
> > > All I know for sure is that we don't want to sink non-dynamic
> > > allocas
> > > out of the entry block.  I can modify the patch to handle only
> > > this case, so that the we retain the old behavior in the other
> > > cases.
> > 
> > I think that would be good; we probably still want to sink dynamic
> > allocas.
> > 
> >  -Hal
> > 
> > > 
> > > -Tom
> > > 
> > > 
> > > 
> > > > 
> > > >  -Hal
> > > > 
> > > > ----- Original Message -----
> > > > > From: "Tom Stellard" <tom at stellard.net>
> > > > > To: "Matt Arsenault" <Matthew.Arsenault at amd.com>
> > > > > Cc: llvm-commits at cs.uiuc.edu, "Tom Stellard"
> > > > > <thomas.stellard at amd.com>
> > > > > Sent: Monday, January 20, 2014 2:53:24 PM
> > > > > Subject: Re: [PATCH] Sink: Don't sink allocas
> > > > > 
> > > > > Looks like I forgot the patch...
> > > > > 
> > > > > On Mon, Jan 20, 2014 at 12:49:23PM -0800, Tom Stellard wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > Attached is an updated patch.
> > > > > > 
> > > > > > -Tom
> > > > > > 
> > > > > > On Mon, Dec 16, 2013 at 10:21:25AM -0800, Matt Arsenault
> > > > > > wrote:
> > > > > > > On 12/16/2013 07:17 AM, Tom Stellard wrote:
> > > > > > > >From: Tom Stellard <thomas.stellard at amd.com>
> > > > > > > >
> > > > > > > >CodeGen treats allocas outside the entry block as
> > > > > > > >dynamically
> > > > > > > >sized
> > > > > > > >stack objects.
> > > > > > > >---
> > > > > > > >  lib/Transforms/Scalar/Sink.cpp |  6 ++++++
> > > > > > > >  test/Transforms/Sink/basic.ll  | 23
> > > > > > > >  +++++++++++++++++++++++
> > > > > > > >  2 files changed, 29 insertions(+)
> > > > > > > >
> > > > > > > >diff --git a/lib/Transforms/Scalar/Sink.cpp
> > > > > > > >b/lib/Transforms/Scalar/Sink.cpp
> > > > > > > >index d4595bb..c7f9e97 100644
> > > > > > > >--- a/lib/Transforms/Scalar/Sink.cpp
> > > > > > > >+++ b/lib/Transforms/Scalar/Sink.cpp
> > > > > > > >@@ -218,6 +218,12 @@ bool
> > > > > > > >Sinking::IsAcceptableTarget(Instruction *Inst,
> > > > > > > >  /// instruction out of its current block into a
> > > > > > > >  successor.
> > > > > > > >  bool Sinking::SinkInstruction(Instruction *Inst,
> > > > > > > >                                SmallPtrSet<Instruction
> > > > > > > >                                *,
> > > > > > > >                                8>
> > > > > > > >                                &Stores) {
> > > > > > > >+
> > > > > > > >+  // Don't sink alloca instructions.  CodeGen assumes
> > > > > > > >allocas
> > > > > > > >outside the
> > > > > > > >+  // entry block are dynamically sized stack objects.
> > > > > > > >+  if (dyn_cast<AllocaInst>(Inst))
> > > > > > > >+    return false;
> > > > > > > >+
> > > > > > > You don't use the value of dyn_cast, so just use isa
> > > > > > > 
> > > > > > > >    // Check if it's safe to move the instruction.
> > > > > > > >    if (!isSafeToMove(Inst, AA, Stores))
> > > > > > > >      return false;
> > > > > > > >diff --git a/test/Transforms/Sink/basic.ll
> > > > > > > >b/test/Transforms/Sink/basic.ll
> > > > > > > >index 85ab376..740e5a8 100644
> > > > > > > >--- a/test/Transforms/Sink/basic.ll
> > > > > > > >+++ b/test/Transforms/Sink/basic.ll
> > > > > > > >@@ -62,3 +62,26 @@ X:
> > > > > > > >                                    ;
> > > > > > > >preds = %5, %3
> > > > > > > >    ret i32 %R
> > > > > > > >  }
> > > > > > > >+; We shouldn't sink allocas, since CodeGen interprets
> > > > > > > >allocas
> > > > > > > >outside the
> > > > > > > >+; entry block as dynamically sized stack objects.
> > > > > > > >+
> > > > > > > >+; CHECK-LABEL: @alloca_sink
> > > > > > > >+; CHECK: entry:
> > > > > > > >+; CHECK-NEXT: alloca
> > > > > > > >+define i32 @alloca_sink(i32 %a, i32 %b) {
> > > > > > > >+entry:
> > > > > > > >+  %0 = alloca i32
> > > > > > > >+  %1 = icmp ne i32 %a, 0
> > > > > > > >+  br i1 %1, label %if, label %endif
> > > > > > > >+
> > > > > > > >+if:
> > > > > > > >+  %2 = getelementptr i32* %0, i32 1
> > > > > > > >+  store i32 0, i32* %0
> > > > > > > >+  store i32 1, i32* %2
> > > > > > > >+  %3 = getelementptr i32* %0, i32 %b
> > > > > > > >+  %4 = load i32* %3
> > > > > > > >+  ret i32 %4
> > > > > > > >+
> > > > > > > >+endif:
> > > > > > > >+  ret i32 0
> > > > > > > >+}
> > > > > > > 
> > > > > > > 
> > > > > > _______________________________________________
> > > > > > llvm-commits mailing list
> > > > > > llvm-commits at cs.uiuc.edu
> > > > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > > > > 
> > > > > 
> > > > > _______________________________________________
> > > > > llvm-commits mailing list
> > > > > llvm-commits at cs.uiuc.edu
> > > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > > > > 
> > > > 
> > > > --
> > > > Hal Finkel
> > > > Assistant Computational Scientist
> > > > Leadership Computing Facility
> > > > Argonne National Laboratory
> > > 
> > 
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory