[PATCH] R600/SI: Fix bug from insertion of llvm.SI.end.cf into loop headers

[Christian König]

In this case patch LGTM.

Regards,
Christian.

Am 04.02.2015 um 16:25 schrieb Tom Stellard:
> On Wed, Feb 04, 2015 at 10:52:08AM +0100, Christian König wrote:
>> Nice catch.
>>
>> Do we have the loop info available anyway? If no just always
>> creating a new block for the end.cf might be more appropriate.
>>
> Loops are actually a special case, because we need to make sure that
> backedges point to the original basic block and not the new block
> for llvm.SI.end.cf, so we still need the loop info or some equivalent
> analysis to identify the backedges.
>
> -Tom
>
>> Regards,
>> Christian.
>>
>> Am 04.02.2015 um 03:07 schrieb Tom Stellard:
>>> The llvm.SI.end.cf intrinsic is used to mark the end of if-then blocks,
>>> if-then-else blocks, and loops.  It is responsible for updating the
>>> exec mask to re-enable threads that had been masked during the preceding
>>> control flow block.  For example:
>>>
>>> s_mov_b64 exec, 0x3                 ; Initial exec mask
>>> s_mov_b64 s[0:1], exec              ; Saved exec mask
>>> v_cmpx_gt_u32 exec, s[2:3], v0, 0   ; llvm.SI.if
>>> do_stuff()
>>> s_or_b64 exec, exec, s[0:1]         ; llvm.SI.end.cf
>>>
>>> The bug fixed by this patch was one where the llvm.SI.end.cf intrinsic
>>> was being inserted into the header of loops.  This would happen when
>>> an if block terminated in a loop header and we would end up with
>>> code like this:
>>>
>>> s_mov_b64 exec, 0x3                 ; Initial exec mask
>>> s_mov_b64 s[0:1], exec              ; Saved exec mask
>>> v_cmpx_gt_u32 exec, s[2:3], v0, 0   ; llvm.SI.if
>>> do_stuff()
>>>
>>> LOOP:                       ; Start of loop header
>>> s_or_b64 exec, exec, s[0:1] ; llvm.SI.end.cf <-BUG: The exec mask has the
>>>                                same value at the beginning of each loop
>>> 			      iteration.
>>> do_stuff();
>>> s_cbranch_execnz LOOP
>>>
>>> The fix is to create a new basic block before the loop and insert the
>>> llvm.SI.end.cf there.  This way the exec mask is restored before the
>>> start of the loop instead of at the beginning of each iteration.
>>> ---
>>>   lib/Target/R600/SIAnnotateControlFlow.cpp | 27 +++++++++++++++++++++---
>>>   test/CodeGen/R600/endcf-loop-header.ll    | 34 +++++++++++++++++++++++++++++++
>>>   2 files changed, 58 insertions(+), 3 deletions(-)
>>>   create mode 100644 test/CodeGen/R600/endcf-loop-header.ll
>>>
>>> diff --git a/lib/Target/R600/SIAnnotateControlFlow.cpp b/lib/Target/R600/SIAnnotateControlFlow.cpp
>>> index 91eb60b..79f6532 100644
>>> --- a/lib/Target/R600/SIAnnotateControlFlow.cpp
>>> +++ b/lib/Target/R600/SIAnnotateControlFlow.cpp
>>> @@ -14,6 +14,7 @@
>>>   #include "AMDGPU.h"
>>>   #include "llvm/ADT/DepthFirstIterator.h"
>>> +#include "llvm/Analysis/LoopInfo.h"
>>>   #include "llvm/IR/Constants.h"
>>>   #include "llvm/IR/Dominators.h"
>>>   #include "llvm/IR/Instructions.h"
>>> @@ -66,6 +67,8 @@ class SIAnnotateControlFlow : public FunctionPass {
>>>     DominatorTree *DT;
>>>     StackVector Stack;
>>> +  LoopInfo *LI;
>>> +
>>>     bool isTopOfStack(BasicBlock *BB);
>>>     Value *popSaved();
>>> @@ -99,6 +102,7 @@ public:
>>>     }
>>>     void getAnalysisUsage(AnalysisUsage &AU) const override {
>>> +    AU.addRequired<LoopInfoWrapperPass>();
>>>       AU.addRequired<DominatorTreeWrapperPass>();
>>>       AU.addPreserved<DominatorTreeWrapperPass>();
>>>       FunctionPass::getAnalysisUsage(AU);
>>> @@ -277,10 +281,26 @@ void SIAnnotateControlFlow::handleLoop(BranchInst *Term) {
>>>     Term->setCondition(CallInst::Create(Loop, Arg, "", Term));
>>>     push(Term->getSuccessor(0), Arg);
>>> -}
>>> -
>>> -/// \brief Close the last opened control flow
>>> +}/// \brief Close the last opened control flow
>>>   void SIAnnotateControlFlow::closeControlFlow(BasicBlock *BB) {
>>> +  llvm::Loop *L = LI->getLoopFor(BB);
>>> +
>>> +  if (L && L->getHeader() == BB) {
>>> +    // We can't insert an EndCF call into a loop header, because it will
>>> +    // get executed on every iteration of the loop, when it should be
>>> +    // executed only once before the loop.
>>> +    SmallVector <BasicBlock*, 8> Latches;
>>> +    L->getLoopLatches(Latches);
>>> +
>>> +    std::vector<BasicBlock*> Preds;
>>> +    for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI != PE; ++PI) {
>>> +      if (std::find(Latches.begin(), Latches.end(), *PI) == Latches.end())
>>> +        Preds.push_back(*PI);
>>> +    }
>>> +    BB = llvm::SplitBlockPredecessors(BB, Preds, "endcf.split", nullptr, DT,
>>> +                                      LI, false);
>>> +  }
>>> +
>>>     CallInst::Create(EndCf, popSaved(), "", BB->getFirstInsertionPt());
>>>   }
>>> @@ -288,6 +308,7 @@ void SIAnnotateControlFlow::closeControlFlow(BasicBlock *BB) {
>>>   /// recognize if/then/else and loops.
>>>   bool SIAnnotateControlFlow::runOnFunction(Function &F) {
>>>     DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
>>> +  LI = &getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
>>>     for (df_iterator<BasicBlock *> I = df_begin(&F.getEntryBlock()),
>>>          E = df_end(&F.getEntryBlock()); I != E; ++I) {
>>> diff --git a/test/CodeGen/R600/endcf-loop-header.ll b/test/CodeGen/R600/endcf-loop-header.ll
>>> new file mode 100644
>>> index 0000000..e3c5b3c
>>> --- /dev/null
>>> +++ b/test/CodeGen/R600/endcf-loop-header.ll
>>> @@ -0,0 +1,34 @@
>>> +; RUN: llc < %s -march=amdgcn -mcpu=SI -verify-machineinstrs | FileCheck %s
>>> +
>>> +; This tests that the llvm.SI.end.cf intrinsic is not inserted into the
>>> +; loop block.  This intrinsic will be lowered to s_or_b64 by the code
>>> +; generator.
>>> +
>>> +; CHECK-LABEL: {{^}}test:
>>> +
>>> +; This is was lowered from the llvm.SI.end.cf intrinsic:
>>> +; CHECK: s_or_b64 exec, exec
>>> +
>>> +; CHECK: [[LOOP_LABEL:[0-9A-Za-z_]+]]: ; %loop{{$}}
>>> +; CHECK-NOT: s_or_b64 exec, exec
>>> +; CHECK: s_cbranch_execnz [[LOOP_LABEL]]
>>> +define void @test(i32 addrspace(1)* %out, i32 %cond) {
>>> +entry:
>>> +  %tmp0 = icmp eq i32 %cond, 0
>>> +  br i1 %tmp0, label %if, label %loop
>>> +
>>> +if:
>>> +  store i32 0, i32 addrspace(1)* %out
>>> +  br label %loop
>>> +
>>> +loop:
>>> +  %tmp1 = phi i32 [0, %entry], [0, %if], [%inc, %loop]
>>> +  %inc = add i32 %tmp1, %cond
>>> +  %tmp2 = icmp ugt i32 %inc, 10
>>> +  br i1 %tmp2, label %done, label %loop
>>> +
>>> +done:
>>> +  %tmp3 = getelementptr i32 addrspace(1)* %out, i64 1
>>> +  store i32 %inc, i32 addrspace(1)* %tmp3
>>> +  ret void
>>> +}
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits