[PATCH] IRBuilder: Allow retrieval of the inserted instructions

[Adam Nemet]

> On Feb 5, 2015, at 1:06 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> ----- Original Message -----
>> From: "Adam Nemet" <anemet at apple.com <mailto:anemet at apple.com>>
>> To: "Hal Finkel" <hfinkel at anl.gov <mailto:hfinkel at anl.gov>>
>> Cc: reviews+D7421+public+86df0334cf13545b at reviews.llvm.org <mailto:reviews+D7421+public+86df0334cf13545b at reviews.llvm.org>, "llvm-commits" <llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>>, "Nadav
>> Rotem(nrotem at apple.com <mailto:nrotem at apple.com>)" <nrotem at apple.com <mailto:nrotem at apple.com>>, "Arnold Schwaighofer" <aschwaighofer at apple.com <mailto:aschwaighofer at apple.com>>, chandlerc at gmail.com <mailto: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 <mailto:hfinkel at anl.gov> > wrote:
>> 
>> 
>> ----- Original Message -----
>> 
>> 
>> From: "Adam Nemet" < anemet at apple.com <mailto:anemet at apple.com> >
>> To: "Hal Finkel" < hfinkel at anl.gov <mailto:hfinkel at anl.gov> >
>> Cc: reviews+D7421+public+86df0334cf13545b at reviews.llvm.org <mailto:reviews+D7421+public+86df0334cf13545b at reviews.llvm.org> ,
>> "llvm-commits" < llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu> >, "Nadav Rotem <
>> nrotem at apple.com <mailto:nrotem at apple.com> > ( nrotem at apple.com <mailto:nrotem at apple.com> )" < nrotem at apple.com <mailto:nrotem at apple.com> >,
>> "Arnold Schwaighofer" < aschwaighofer at apple.com <mailto:aschwaighofer at apple.com> >,
>> chandlerc at gmail.com <mailto: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 <mailto: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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150205/5d2029b0/attachment.html>