[PATCH] Check for all known bits on ret in InstCombine

Hal Finkel hfinkel at anl.gov
Tue Jul 22 10:25:05 PDT 2014


----- Original Message -----
> From: "Philip Reames" <listmail at philipreames.com>
> To: reviews+D4567+public+8b075aecf68909a0 at reviews.llvm.org, hfinkel at anl.gov, chandlerc at gmail.com
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Tuesday, July 22, 2014 12:19:56 PM
> Subject: Re: [PATCH] Check for all known bits on ret in InstCombine
> 
> 
> The code change LGTM.
> 
> The changes inspires two thoughts:
> 1) As we've discussed off list and Chandler mentioned, we need to
> investigate profitability of various invariant condition propagation
> schemes.

Agreed.

> 2) When do we remove a llvm.assume? In your test case, you had an
> assume which (after the transform) retained a condition about an
> otherwise used value (%a). This seems like an obvious case to remove
> since we've gotten all the benefit we can from it. Are there other
> cases like this we should implement?

No, I disagree. We don't want to remove the invariants until after inlining is complete. And for alignment guarantees, for example, we don't want to remove them until after vectorization and unrolling. Currently, they're removed on entry to CodeGen, and it is not clear to me that we need or want to do anything else.

> 
> p.s. Do you have a wiki or notes page somewhere where we can track
> potential enhancements? I worry large parts are getting lost in all
> the various review threads.

No, at least not currently. Although you can construct a 'custom query' on the reviews.llvm.org page that just shows you my active patches (you can even change the sorting order by last-updated time). I have one of these ;)

Thanks again,
Hal

> 
> Philip
> 
> On 07/17/2014 10:58 AM, hfinkel at anl.gov wrote:
> 
> 
> Hi chandlerc,
> 
> From a combination of invariants (and perhaps through other means),
> it is possible that all bits of a return value might be known.
> Currently, InstCombine does not check for this (which is
> understandable given assumptions of constant propagation), but means
> that we'd miss simple cases where invariants are involved.
> http://reviews.llvm.org/D4567 Files:
>   lib/Transforms/InstCombine/InstCombine.h
>   lib/Transforms/InstCombine/InstructionCombining.cpp
>   test/Transforms/InstCombine/invariants.ll
> 
> Index: lib/Transforms/InstCombine/InstCombine.h
> ===================================================================
> --- lib/Transforms/InstCombine/InstCombine.h
> +++ lib/Transforms/InstCombine/InstCombine.h
> @@ -217,6 +217,7 @@
>    Instruction *visitStoreInst(StoreInst &SI);
>    Instruction *visitBranchInst(BranchInst &BI);
>    Instruction *visitSwitchInst(SwitchInst &SI);
> +  Instruction *visitReturnInst(ReturnInst &RI);
>    Instruction *visitInsertValueInst(InsertValueInst &IV);
>    Instruction *visitInsertElementInst(InsertElementInst &IE);
>    Instruction *visitExtractElementInst(ExtractElementInst &EI);
> Index: lib/Transforms/InstCombine/InstructionCombining.cpp
> ===================================================================
> --- lib/Transforms/InstCombine/InstructionCombining.cpp
> +++ lib/Transforms/InstCombine/InstructionCombining.cpp
> @@ -1923,7 +1923,25 @@
>    return nullptr;
>  }
>  
> +Instruction *InstCombiner::visitReturnInst(ReturnInst &RI) {
> +  if (RI.getNumOperands() == 0) // ret void
> +    return nullptr;
> +
> +  Value *ResultOp = RI.getOperand(0);
> +  Type *VTy = ResultOp->getType();
> +  if (!VTy->isIntegerTy())
> +    return nullptr;
>  
> +  // There might be invariants dominating this return that
> completely determine
> +  // the value. If so, constant fold it.
> +  unsigned BitWidth = VTy->getPrimitiveSizeInBits();
> +  APInt KnownZero(BitWidth, 0), KnownOne(BitWidth, 0);
> +  computeKnownBits(ResultOp, KnownZero, KnownOne, 0, &RI);
> +  if ((KnownZero|KnownOne).isAllOnesValue())
> +    RI.setOperand(0, Constant::getIntegerValue(VTy, KnownOne));
> +
> +  return nullptr;
> +}
>  
>  Instruction *InstCombiner::visitBranchInst(BranchInst &BI) {
>    // Change br (not X), label True, label False to: br X, label
>    False, True
> Index: test/Transforms/InstCombine/invariants.ll
> ===================================================================
> --- test/Transforms/InstCombine/invariants.ll
> +++ test/Transforms/InstCombine/invariants.ll
> @@ -43,6 +43,18 @@
>  ; Function Attrs: nounwind
>  declare void @llvm.invariant(i1) #1
>  
> +define i32 @simple(i32 %a) #1 {
> +entry:
> +
> +; CHECK-LABEL: @simple
> +; CHECK: call void @llvm.invariant
> +; CHECK: ret i32 4
> +
> +  %cmp = icmp eq i32 %a, 4
> +  tail call void @llvm.invariant(i1 %cmp)
> +  ret i32 %a
> +}
> +
>  ; Function Attrs: nounwind uwtable
>  define i32 @bar1(i32 %a) #0 {
>  entry:
> 
> _______________________________________________
> llvm-commits mailing list llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory



More information about the llvm-commits mailing list