[PATCH] IRBuilder: Allow retrieval of the inserted instructions

[Hal Finkel]

----- Original Message -----
> From: "Adam Nemet" <anemet at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: reviews+D7421+public+86df0334cf13545b at reviews.llvm.org, "llvm-commits" <llvm-commits at cs.uiuc.edu>, "Nadav
> Rotem(nrotem at apple.com)" <nrotem at apple.com>, "Arnold Schwaighofer" <aschwaighofer at apple.com>, chandlerc at gmail.com
> Sent: Thursday, February 5, 2015 2:08:33 PM
> Subject: Re: [PATCH] IRBuilder: Allow retrieval of the inserted instructions
> 
> On Feb 5, 2015, at 1:06 AM, Hal Finkel < hfinkel at anl.gov > wrote:
> 
> ----- Original Message -----
> 
> 
> From: "Adam Nemet" < anemet at apple.com >
> To: "Hal Finkel" < hfinkel at anl.gov >
> Cc: reviews+D7421+public+86df0334cf13545b at reviews.llvm.org ,
> "llvm-commits" < llvm-commits at cs.uiuc.edu >, "Nadav
> Rotem( nrotem at apple.com )" < nrotem at apple.com >, "Arnold
> Schwaighofer" < aschwaighofer at apple.com >, chandlerc at gmail.com
> Sent: Thursday, February 5, 2015 2:19:24 AM
> Subject: Re: [PATCH] IRBuilder: Allow retrieval of the inserted
> instructions
> 
> On Feb 4, 2015, at 10:15 PM, Hal Finkel < hfinkel at anl.gov > wrote:
> 
> 
> ----- Original Message -----
> 
> 
> From: "Adam Nemet" < anemet at apple.com >
> To: "Hal Finkel" < hfinkel at anl.gov >
> Cc: reviews+D7421+public+86df0334cf13545b at reviews.llvm.org ,
> "llvm-commits" < llvm-commits at cs.uiuc.edu >, "Nadav Rotem <
> nrotem at apple.com > ( nrotem at apple.com )" < nrotem at apple.com >,
> "Arnold Schwaighofer" < aschwaighofer at apple.com >,
> chandlerc at gmail.com
> Sent: Wednesday, February 4, 2015 10:04:54 PM
> Subject: Re: [PATCH] IRBuilder: Allow retrieval of the inserted
> instructions
> 
> On Feb 4, 2015, at 6:14 PM, Hal Finkel < hfinkel at anl.gov > wrote:
> 
> ----- Original Message -----
> 
> From: "Adam Nemet" < anemet at apple.com >
> To: anemet at apple.com , hfinkel at anl.gov , nrotem at apple.com ,
> aschwaighofer at apple.com , chandlerc at gmail.com
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Wednesday, February 4, 2015 5:58:45 PM
> Subject: [PATCH] IRBuilder: Allow retrieval of the inserted
> instructions
> 
> Hi hfinkel, nadav, aschwaighofer, chandlerc,
> 
> This is an RFC.
> 
> When the Loop Vectorizer builds the instruction sequence for checks
> it tries
> to determine the first instruction that was emitted in the current
> block.
> This is then used to split the block.
> 
> Silly question: Why don't we split the block first?
> 
> 
> 
> Because we may end up not needing any checks in which case we don’t
> split. The decision of whether checks are necessary is local to both
> addRuntimeCheck and addStrideCheck.
> Okay, but an alternative design would be to always split early, and
> then let SimplifyCFG put them back together again should we not
> actually end up with any checks, right? I'm not necessarily
> advocating for this approach, and think that the marker code will be
> cleaner than the existing getFirstInst code, but splitting the block
> eagerly will also create a trivial way of marking that point, and I
> want to understand if there are any downsides to that approach
> before we invent something new.
> 
> 
> 
> I am not aware of any other downsides but I haven’t written this code
> of course. Hopefully Nadav/Arnold will chime in if there is
> something to add here.
> 
> 
> Perhaps as another argument you could consider that there seems to be
> some existing practice to fix up simple things like this in the pass
> itself rather than relying on other passes. Especially in this case
> where we could avoid producing the potential inefficiency.
> 
> 
> You're certainly right about that. And I certainly have no problems
> with this so long as it remains embedded inside the transformation
> itself.
> 
> 
> 
> 
> I guess I am also interested if you see some gotchas with the marker
> feature or you just think that it’s not a generally useful new
> addition.
> 
> I'm not sure that I'm seeing the complete picture. Are these going to
> be on the new interface boundary between the vectorization analysis
> utilities and their users? Or just used internally?
> 
> 
> 
> getFirstInst is used by both addRuntimeCheck and addStrideCheck. I’d
> like to move addRuntimeCheck to LAA but not stride checking. Stride
> checking has some more dependencies that makes this a bit more
> involved. So getFirstInst or the corresponding feature would have to
> live somewhere where it could be shared between to LV.cpp and
> LAA.cpp.

FWIW, it is just a few lines of fairly generic code in a static function. I'd not object to copy-and-paste for now while we figure out what we actually want to do here.

 -Hal

> 
> 
> I hope this clears up the picture.
> 
> 
> 
> Any why is this marker class associated with IRBuilder?
> 
> 
> 
> I doesn’t have to. It’s actually associated with an insertion point.
> I put it in the IRBuilder header because:
> 
> 
> 1. I felt that insertion point was an IRBuilder concept
> 2. Its main aim is to query the result of IRBuilder inserts that may
> or may not end up generating instructions
> 3. I was thinking that it’s a generally useful feature essentially
> answering the question of what IRBuilder has actually done with the
> instruction stream through its various levels of smartness
> 
> 
> We can obviously move it under LAA or as you say rip it out
> completely and have SimplifyCFG fix this up for us. I was mostly
> trying to test the waters if this was a generally interesting
> addition.
> 
> 
> Sounds like it’s not. I can try to see if there are code changes
> without getFirstInst and report back.
> 
> 
> Adam
> 
> 
> 
> Thanks again,
> Hal
> 
> 
> 
> 
> 
> Thanks,
> Adam
> 
> Thanks again,
> Hal
> 
> Adam
> 
> 
> 
> -Hal
> 
> It uses a custom solution for this implemented in the static function
> getFirstInst. The pattern is something like:
> 
> Value *V = IRBuilder.CreateBlah(...);
> FirstInst = getFirstInst(FirstInst, V, Loc)
> Value *V2 = IRBuilder.CreateBlah(...);
> FirstInst = getFirstInst(FirstInst, V2, Loc);
> 
> (Since CreateBlah may return a constant we may not generate the first
> instruction for V.)
> 
> --
> 
> 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