[PATCH] IRBuilder: Allow retrieval of the inserted instructions

[Adam Nemet]

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

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.)
>> 
>> For the LoopAccessAnalysis work I need this to be made global so I
>> was
>> thinking how to make it more generic.  My idea was to change the
>> approach and
>> rather than repeatedly checking whether we had emitted the first
>> instruction,
>> remember the predecessor of the insertion point in the IRBuilder.
>> Subsequently, when the first emitted instruction is queried I return
>> the
>> successor of the saved predecessor instruction.
>> 
>> Conceptually:
>> 
>> IRBuilder<>::Marker Mark(IRBuilder.getInsertPoint());
>> 
>> marks the next instruction that will be inserted at the current
>> insertion
>> point.  After inserting instructions into IRBuilder, you can retrieve
>> the
>> first instruction inserted with Mark.get() (perhaps Mark.first()
>> would be a
>> better name).
>> 
>> The patch also contains the changes to make use of this in LV and
>> then asserts
>> that the new result is at least as good as the one from getFirstInst.
>> The
>> assert does not fail on things I tried so far.
>> 
>> I went back and forth on this but I think it's better to associate
>> the marker
>> with an insertion point rather than the IRBuilder instance itself.  I
>> had two
>> reasons to go this way:
>> 
>> 1. The insertion point can be adjusted manually in the IRBuilder.
>> This makes
>> it more obvious that the marker is associated with the original
>> insertion
>> point after a change.
>> 
>> 2. The behavior is more intuitive with nested IRBuilders.
>> 
>> For example in the LV code that I am changing we should really move
>> the marker
>> earlier to also cover the SCEV expansion of the array bounds.  Those
>> instructions are however inserted via an implicit IRBuilder inside
>> SCEV
>> expansion.  The good thing is that the marker as written sees these
>> because of
>> the shared insertion point.
>> 
>> The patch is still missing unit tests but wanted to float the idea
>> before I
>> get too carried away.  Is this a reasonable idea?  Are there other
>> issues I am
>> missing?
>> 
>> http://reviews.llvm.org/D7421
>> 
>> Files:
>>  include/llvm/IR/IRBuilder.h
>>  lib/Transforms/Vectorize/LoopVectorize.cpp
>> 
>> Index: include/llvm/IR/IRBuilder.h
>> ===================================================================
>> --- include/llvm/IR/IRBuilder.h
>> +++ include/llvm/IR/IRBuilder.h
>> @@ -188,6 +188,24 @@
>>   /// \brief Set the fast-math flags to be used with generated
>>   fp-math operators
>>   void SetFastMathFlags(FastMathFlags NewFMF) { FMF = NewFMF; }
>> 
>> +  class Marker {
>> +    BasicBlock::iterator Prev;
>> +    BasicBlock *BB;
>> +  public:
>> +    Marker(BasicBlock::iterator InsertionPoint) : BB(nullptr) {
>> +      if (InsertionPoint == InsertionPoint->getParent()->begin())
>> +        BB = InsertionPoint->getParent();
>> +      else
>> +        Prev = std::prev(InsertionPoint);
>> +    }
>> +    BasicBlock::iterator get() const {
>> +      if (BB)
>> +        return BB->begin();
>> +      else
>> +        return std::next(Prev);
>> +    }
>> +  };
>> +
>>   //===--------------------------------------------------------------------===//
>>   // RAII helpers.
>>   //===--------------------------------------------------------------------===//
>> Index: lib/Transforms/Vectorize/LoopVectorize.cpp
>> ===================================================================
>> --- lib/Transforms/Vectorize/LoopVectorize.cpp
>> +++ lib/Transforms/Vectorize/LoopVectorize.cpp
>> @@ -2050,6 +2050,8 @@
>>   }
>> 
>>   IRBuilder<> ChkBuilder(Loc);
>> +  IRBuilder<>::Marker BeforeInsertionMarker(Loc);
>> +
>>   // Our instructions might fold to a constant.
>>   Value *MemoryRuntimeCheck = nullptr;
>>   for (unsigned i = 0; i < NumPointers; ++i) {
>> @@ -2102,6 +2104,14 @@
>>                                                  ConstantInt::getTrue(Ctx));
>>   ChkBuilder.Insert(Check, "memcheck.conflict");
>>   FirstInst = getFirstInst(FirstInst, Check, Loc);
>> +
>> +  Instruction *AnotherFirst = BeforeInsertionMarker.get();
>> +  assert(std::find_if(BasicBlock::iterator(AnotherFirst),
>> +                      Loc->getParent()->end(),
>> +                      [&](Instruction &I) {
>> +                        return &I == FirstInst;
>> +                      }) !=  Loc->getParent()->end());
>> +
>>   return std::make_pair(FirstInst, Check);
>> }
>> 
>> EMAIL PREFERENCES
>>  http://reviews.llvm.org/settings/panel/emailpreferences/
>> 
> 
> -- 
> 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/20150204/728c9dcc/attachment.html>