[polly] r248688 - BlockGenerator: Be less agressive with deleting dead instructions

Sun Sep 27 16:39:22 PDT 2015

Why are we deleting stuff anyway? Even for the current block this seems
risky. I would go the other direction and only create what we need,
e.g., create all accesses in the statement by recursively creating the
operands. Then we should not generate anything that is not
needed and everything that is needed should be (transitively) referenced
by a access (or do I miss something?).

On 09/27, Tobias Grosser via llvm-commits wrote:
> Author: grosser
> Date: Sun Sep 27 14:50:16 2015
> New Revision: 248688
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=248688&view=rev
> Log:
> BlockGenerator: Be less agressive with deleting dead instructions
> 
> We now only delete trivially dead instructions in the BB we copy (copyBB), but
> not in any other BB.  Only for copyBB we know that there will _never_ be any
> future uses of instructions that have no use after copyBB has been generated.
> Other instructions in the AST that have been generated by IslNodeBuilder may
> look dead at the moment, but may possibly still be referenced by GlobalMaps. If
> we delete them now, later uses would break surprisingly.
> 
> We do not have a test case that breaks due to us deleting too many instructions.
> This issue was found by inspection.
> 
> Modified:
>     polly/trunk/lib/CodeGen/BlockGenerators.cpp
>     polly/trunk/test/Isl/CodeGen/MemAccess/codegen_address_space.ll
>     polly/trunk/test/Isl/CodeGen/non-affine-phi-node-expansion-3.ll
> 
> Modified: polly/trunk/lib/CodeGen/BlockGenerators.cpp
> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/lib/CodeGen/BlockGenerators.cpp?rev=248688&r1=248687&r2=248688&view=diff
> ==============================================================================
> --- polly/trunk/lib/CodeGen/BlockGenerators.cpp (original)
> +++ polly/trunk/lib/CodeGen/BlockGenerators.cpp Sun Sep 27 14:50:16 2015
> @@ -285,6 +285,28 @@ void BlockGenerator::copyInstruction(Sco
>    copyInstScalar(Stmt, Inst, BBMap, LTS);
>  }
>  
> +/// @brief Remove trivially dead instructions from BB
> +///
> +/// This function drops trivially dead instructions from a basic block. It
> +/// on purpose does _not_ recurse into other BBs even if the deletion of
> +/// instructions in this basic block can make instructions in other basic blocks
> +/// triviall dead.
> +static void simplifyInstsInBlockOnly(BasicBlock *BB) {
> +  auto BI = --BB->end(), BE = BB->begin();
> +  bool Exit = false;
> +  while (!Exit) {
> +    auto ToRemove = BI;
> +    if (BI != BE)
> +      BI--;
> +    else
> +      Exit = true;
> +
> +    if (!isInstructionTriviallyDead(ToRemove))
> +      continue;
> +    ToRemove->eraseFromParent();
> +  }
> +}
> +
>  void BlockGenerator::copyStmt(ScopStmt &Stmt, LoopToScevMapT &LTS,
>                                isl_id_to_ast_expr *NewAccesses) {
>    assert(Stmt.isBlockStmt() &&
> @@ -296,7 +318,13 @@ void BlockGenerator::copyStmt(ScopStmt &
>    copyBB(Stmt, BB, BBMap, LTS, NewAccesses);
>  
>    auto CopyBB = Builder.GetInsertBlock();
> -  SimplifyInstructionsInBlock(CopyBB, nullptr);
> +  // Delete trivially dead instructions in CopyBB, but not in any other BB.
> +  // Only for copyBB we know that there will _never_ be any future uses of
> +  // instructions that have no use after copyBB has finished. Other instructions
> +  // in the AST that have been generated by IslNodeBuilder may look dead at
> +  // the moment, but may possibly still be referenced by GlobalMaps. If we
> +  // delete them now, later uses would break surprisingly.
> +  simplifyInstsInBlockOnly(CopyBB);
>    Builder.SetInsertPoint(CopyBB->getTerminator());
>  }
>  
> @@ -1090,8 +1118,14 @@ void RegionGenerator::copyStmt(ScopStmt
>      LTS[L] = SE.getUnknown(LoopPHI);
>    }
>  
> +  // Delete trivially dead instructions in CopyBB, but not in any other BB.
> +  // Only for copyBB we know that there will _never_ be any future uses of
> +  // instructions that have no use after copyBB has finished. Other instructions
> +  // in the AST that have been generated by IslNodeBuilder may look dead at
> +  // the moment, but may possibly still be referenced by GlobalMaps. If we
> +  // delete them now, later uses would break surprisingly.
>    for (auto *BB : SeenBlocks)
> -    SimplifyInstructionsInBlock(BlockMap[BB], nullptr);
> +    simplifyInstsInBlockOnly(BlockMap[BB]);
>  
>    // Reset the old insert point for the build.
>    Builder.SetInsertPoint(ExitBBCopy->begin());
> 
> Modified: polly/trunk/test/Isl/CodeGen/MemAccess/codegen_address_space.ll
> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/test/Isl/CodeGen/MemAccess/codegen_address_space.ll?rev=248688&r1=248687&r2=248688&view=diff
> ==============================================================================
> --- polly/trunk/test/Isl/CodeGen/MemAccess/codegen_address_space.ll (original)
> +++ polly/trunk/test/Isl/CodeGen/MemAccess/codegen_address_space.ll Sun Sep 27 14:50:16 2015
> @@ -40,4 +40,5 @@ for.end:
>  }
>  
>  ; CHECK: %polly.access.cast.A = bitcast [100 x i32] addrspace(5)* %A to i32 addrspace(5)*
> -; CHECK: %tmp2_p_scalar_ = load i32, i32 addrspace(5)* %polly.access.cast.A, align 4, !alias.scope !0, !noalias !2
> +; CHECK: %polly.access.A = getelementptr i32, i32 addrspace(5)* %polly.access.cast.A, i64 0
> +; CHECK: %tmp2_p_scalar_ = load i32, i32 addrspace(5)* %polly.access.A, align 4, !alias.scope !0, !noalias !2
> 
> Modified: polly/trunk/test/Isl/CodeGen/non-affine-phi-node-expansion-3.ll
> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/test/Isl/CodeGen/non-affine-phi-node-expansion-3.ll?rev=248688&r1=248687&r2=248688&view=diff
> ==============================================================================
> --- polly/trunk/test/Isl/CodeGen/non-affine-phi-node-expansion-3.ll (original)
> +++ polly/trunk/test/Isl/CodeGen/non-affine-phi-node-expansion-3.ll Sun Sep 27 14:50:16 2015
> @@ -13,9 +13,12 @@ loop:
>    br i1 %cond0, label %branch1, label %backedge
>  
>  ; CHECK-LABEL: polly.stmt.loop:
> -; CHECK-NEXT:    store float 3.000000e+00, float* %merge.phiops
> -; CHECK-NEXT:    store float 3.000000e+00, float* %val1.s2a
> -; CHECK-NEXT:    store float 3.000000e+00, float* %val2.s2a
> +; CHECK-NEXT: %p_val0 = fadd float 1.000000e+00, 2.000000e+00
> +; CHECK-NEXT: %p_val1 = fadd float 1.000000e+00, 2.000000e+00
> +; CHECK-NEXT: %p_val2 = fadd float 1.000000e+00, 2.000000e+00
> +; CHECK-NEXT: store float %p_val0, float* %merge.phiops
> +; CHECK-NEXT: store float %p_val1, float* %val1.s2a
> +; CHECK-NEXT: store float %p_val2, float* %val2.s2a
>  
>  ; FIXME -> The last two writes are not really needed and can be dropped if the
>  ;          incoming block of the PHI and the value that is used share the same
> @@ -25,13 +28,13 @@ branch1:
>    br i1 %cond1, label %branch2, label %backedge
>  
>  ; CHECK-LABEL: polly.stmt.branch1:
> -; CHECK-NEXT:    store float 3.000000e+00, float* %merge.phiops
> +; CHECK-NEXT:    store float %p_val1, float* %merge.phiops
>  
>  branch2:
>    br label %backedge
>  
>  ; CHECK-LABEL: polly.stmt.branch2:
> -; CHECK-NEXT:    store float 3.000000e+00, float* %merge.phiops
> +; CHECK-NEXT:    store float %p_val2, float* %merge.phiops
>  
>  backedge:
>    %merge = phi float [%val0, %loop], [%val1, %branch1], [%val2, %branch2]
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-- 

Johannes Doerfert
Researcher / PhD Student

Compiler Design Lab (Prof. Hack)
Saarland University, Computer Science
Building E1.3, Room 4.31

Tel. +49 (0)681 302-57521 : doerfert at cs.uni-saarland.de
Fax. +49 (0)681 302-3065  : http://www.cdl.uni-saarland.de/people/doerfert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150928/dbd466a3/attachment.sig>