[llvm-commits] [llvm] r111665 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp test/Transforms/InstCombine/2010-08-19-StoreNarrowing.ll

[Chris Lattner]

On Aug 20, 2010, at 11:24 AM, Owen Anderson wrote:

> Author: resistor
> Date: Fri Aug 20 13:24:43 2010
> New Revision: 111665
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=111665&view=rev
> Log:
> Re-apply r111568 with a fix for the clang self-host.

Alright.  Please include a useful commit message though, so I don't have to go look at r111568 to find out what this does.  Some thoughts:

> @@ -473,6 +475,51 @@
> 
>   if (SI.isVolatile()) return 0;  // Don't hack volatile stores.
> 
> +  // Attempt to narrow sequences where we load a wide value, perform bitmasks
> +  // that only affect the low bits of it, and then store it back.  This 
> +  // typically arises from bitfield initializers in C++.

Why is this better to do in instcombine than dagcombine?  Unless the code is already type-unsafe, we typically do *not* want to do this in the mid-level.  In your specific testcase, there is a bitcast of the pointer and you're actually *improving* type safety, but there is nothing in the xform that encourages this.

> +  ConstantInt *CI1 =0, *CI2 = 0;
> +  Value *Ld = 0;
> +  if (getTargetData() &&
> +      match(SI.getValueOperand(),
> +            m_And(m_Or(m_Value(Ld), m_ConstantInt(CI1)), m_ConstantInt(CI2))) &&
> +      isa<LoadInst>(Ld) &&
> +      equivalentAddressValues(cast<LoadInst>(Ld)->getPointerOperand(), Ptr)) {

I don't see where you check to make sure you don't have something like:

x = load P
store 123 ->  Q    ;; might alias P!
x' = stuff(x)
store x' -> P

How do you know the loaded value hasn't been changed in memory?  Also, you don't check for a volatile load here.

> +    APInt OrMask = CI1->getValue();
> +    APInt AndMask = CI2->getValue();

No need to copy the APInt's.

> +    // Compute the prefix of the value that is unmodified by the bitmasking.
> +    unsigned LeadingAndOnes = AndMask.countLeadingOnes();
> +    unsigned LeadingOrZeros = OrMask.countLeadingZeros();
> +    unsigned Prefix = std::min(LeadingAndOnes, LeadingOrZeros);
> +    uint64_t NewWidth = AndMask.getBitWidth() - Prefix;
> +    while (NewWidth < AndMask.getBitWidth() &&
> +           getTargetData()->isIllegalInteger(NewWidth))
> +      NewWidth = NextPowerOf2(NewWidth);

It would be cleaner to just use TD directly instead of calling getTargetData() all over.

> +    // If we can find a power-of-2 prefix (and if the values we're working with
> +    // are themselves POT widths), then we can narrow the store.  We rely on
> +    // later iterations of instcombine to propagate the demanded bits to narrow
> +    // the other computations in the chain.
> +    if (NewWidth < AndMask.getBitWidth() &&
> +        getTargetData()->isLegalInteger(NewWidth)) {
> +      const Type *NewType = IntegerType::get(Ptr->getContext(), NewWidth);
> +      const Type *NewPtrType = PointerType::getUnqual(NewType);

This pointer type needs to be in the same address space as the original load/store.

> +++ llvm/trunk/test/Transforms/InstCombine/2010-08-19-StoreNarrowing.ll Fri Aug 20 13:24:43 2010

This should be folded into an existing testcase, and should not be dated.

> @@ -0,0 +1,21 @@
> +; RUN: opt -S -instcombine %s | not grep and

This should use filecheck.  Have you looked at what instcombine turns this into?  I suspect it isn't what you're expecting.

> +target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
> +target triple = "x86_64-apple-darwin10.0.0"
> +
> +%class.A = type { i8, [3 x i8] }
> +
> +define void @_ZN1AC2Ev(%class.A* %this) nounwind ssp align 2 {

This should not use a mangled name.

-Chris

> +entry:
> +  %0 = bitcast %class.A* %this to i32*            ; <i32*> [#uses=5]
> +  %1 = load i32* %0, align 4                      ; <i32> [#uses=1]
> +  %2 = and i32 %1, -8                             ; <i32> [#uses=2]
> +  store i32 %2, i32* %0, align 4
> +  %3 = and i32 %2, -57                            ; <i32> [#uses=1]
> +  %4 = or i32 %3, 8                               ; <i32> [#uses=2]
> +  store i32 %4, i32* %0, align 4
> +  %5 = and i32 %4, -65                            ; <i32> [#uses=2]
> +  store i32 %5, i32* %0, align 4
> +  %6 = and i32 %5, -129                           ; <i32> [#uses=1]
> +  store i32 %6, i32* %0, align 4
> +  ret void
> +}
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits