[PATCH] IRBuilder: Allow retrieval of the inserted instructions

[Adam Nemet]

> On Feb 5, 2015, at 12:12 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>, "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.

Makes sense.  Thanks for your feedback!

Adam

> 
> -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