From: Daniel Dunbar Date: Wed, 19 Nov 2008 09:36:46 +0000 (+0000) Subject: Fix redundant load of bit-fields on assignment (to get the updated X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=ed3849b456d64d124bddc7ac044f3ce590bd9d69;p=clang Fix redundant load of bit-fields on assignment (to get the updated value). - Use extra argument to EmitStoreThroughLValue to provide place to write update bit-field value if caller requires it. - This fixes several FIXMEs. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@59615 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/CodeGen/CGExpr.cpp b/lib/CodeGen/CGExpr.cpp index 95733abbab..9328a53567 100644 --- a/lib/CodeGen/CGExpr.cpp +++ b/lib/CodeGen/CGExpr.cpp @@ -392,7 +392,8 @@ void CodeGenFunction::EmitStoreThroughLValue(RValue Src, LValue Dst, } void CodeGenFunction::EmitStoreThroughBitfieldLValue(RValue Src, LValue Dst, - QualType Ty) { + QualType Ty, + llvm::Value **Result) { unsigned StartBit = Dst.getBitfieldStartBit(); unsigned BitfieldSize = Dst.getBitfieldSize(); llvm::Value *Ptr = Dst.getBitfieldAddr(); @@ -403,12 +404,31 @@ void CodeGenFunction::EmitStoreThroughBitfieldLValue(RValue Src, LValue Dst, // Get the new value, cast to the appropriate type and masked to // exactly the size of the bit-field. - llvm::Value *NewVal = Src.getScalarVal(); - NewVal = Builder.CreateIntCast(NewVal, EltTy, false, "tmp"); + llvm::Value *SrcVal = Src.getScalarVal(); + llvm::Value *NewVal = Builder.CreateIntCast(SrcVal, EltTy, false, "tmp"); llvm::Constant *Mask = llvm::ConstantInt::get(llvm::APInt::getLowBitsSet(EltTySize, BitfieldSize)); NewVal = Builder.CreateAnd(NewVal, Mask, "bf.value"); + // Return the new value of the bit-field, if requested. + if (Result) { + // Cast back to the proper type for result. + const llvm::Type *SrcTy = SrcVal->getType(); + llvm::Value *SrcTrunc = Builder.CreateIntCast(NewVal, SrcTy, false, + "bf.reload.val"); + + // Sign extend if necessary. + if (Dst.isBitfieldSigned()) { + unsigned SrcTySize = CGM.getTargetData().getTypeSizeInBits(SrcTy); + llvm::Value *ExtraBits = llvm::ConstantInt::get(SrcTy, + SrcTySize - BitfieldSize); + SrcTrunc = Builder.CreateAShr(Builder.CreateShl(SrcTrunc, ExtraBits), + ExtraBits, "bf.reload.sext"); + } + + *Result = SrcTrunc; + } + // In some cases the bitfield may straddle two memory locations. // Emit the low part first and check to see if the high needs to be // done. diff --git a/lib/CodeGen/CGExprScalar.cpp b/lib/CodeGen/CGExprScalar.cpp index d87edc3e7b..b236a6ad31 100644 --- a/lib/CodeGen/CGExprScalar.cpp +++ b/lib/CodeGen/CGExprScalar.cpp @@ -786,15 +786,14 @@ Value *ScalarExprEmitter::EmitCompoundAssign(const CompoundAssignOperator *E, // Convert the result back to the LHS type. Result = EmitScalarConversion(Result, ResultTy, LHSTy); - // Store the result value into the LHS lvalue. - CGF.EmitStoreThroughLValue(RValue::get(Result), LHSLV, LHSTy); - - // For bitfields, we need the value in the bitfield. Note that - // property references do not reload their value (even though the - // setter may have changed it). - // FIXME: This adds an extra bitfield load + // Store the result value into the LHS lvalue. Bit-fields are + // handled specially because the result is altered by the store. if (LHSLV.isBitfield()) - Result = EmitLoadOfLValue(LHSLV, LHSTy); + CGF.EmitStoreThroughBitfieldLValue(RValue::get(Result), LHSLV, LHSTy, + &Result); + else + CGF.EmitStoreThroughLValue(RValue::get(Result), LHSLV, LHSTy); + return Result; } @@ -1003,16 +1002,14 @@ Value *ScalarExprEmitter::VisitBinAssign(const BinaryOperator *E) { LValue LHS = EmitLValue(E->getLHS()); Value *RHS = Visit(E->getRHS()); - // Store the value into the LHS. + // Store the value into the LHS. Bit-fields are handled specially + // because the result is altered by the store. // FIXME: Volatility! - CGF.EmitStoreThroughLValue(RValue::get(RHS), LHS, E->getType()); - - // For bitfields, we need the value in the bitfield. Note that - // property references do not reload their value (even though the - // setter may have changed it). - // FIXME: This adds an extra bitfield load if (LHS.isBitfield()) - return EmitLoadOfLValue(LHS, E->getLHS()->getType()); + CGF.EmitStoreThroughBitfieldLValue(RValue::get(RHS), LHS, E->getType(), + &RHS); + else + CGF.EmitStoreThroughLValue(RValue::get(RHS), LHS, E->getType()); // Return the RHS. return RHS; diff --git a/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h index 5d99dba69a..05b2793456 100644 --- a/lib/CodeGen/CodeGenFunction.h +++ b/lib/CodeGen/CodeGenFunction.h @@ -32,6 +32,7 @@ namespace llvm { class BasicBlock; class Module; class SwitchInst; + class Value; } namespace clang { @@ -446,8 +447,16 @@ public: void EmitStoreThroughLValue(RValue Src, LValue Dst, QualType Ty); void EmitStoreThroughExtVectorComponentLValue(RValue Src, LValue Dst, QualType Ty); - void EmitStoreThroughBitfieldLValue(RValue Src, LValue Dst, QualType Ty); void EmitStoreThroughPropertyRefLValue(RValue Src, LValue Dst, QualType Ty); + + /// EmitStoreThroughLValue - Store Src into Dst with same + /// constraints as EmitStoreThroughLValue. + /// + /// \param Result [out] - If non-null, this will be set to a Value* + /// for the bit-field contents after the store, appropriate for use + /// as the result of an assignment to the bit-field. + void EmitStoreThroughBitfieldLValue(RValue Src, LValue Dst, QualType Ty, + llvm::Value **Result=0); // Note: only availabe for agg return types LValue EmitBinaryOperatorLValue(const BinaryOperator *E); diff --git a/lib/CodeGen/README.txt b/lib/CodeGen/README.txt index 33fbfdb744..12a08e161b 100644 --- a/lib/CodeGen/README.txt +++ b/lib/CodeGen/README.txt @@ -17,8 +17,3 @@ appropriately aligned then is is a lot shorter to just load the char directly. //===---------------------------------------------------------------------===// - -Bitfields should not reload the stored value just to return the -correct result. - -//===---------------------------------------------------------------------===// diff --git a/test/CodeGen/bitfield-assign.c b/test/CodeGen/bitfield-assign.c new file mode 100644 index 0000000000..fc163b4d3b --- /dev/null +++ b/test/CodeGen/bitfield-assign.c @@ -0,0 +1,44 @@ +/* Check that the result of a bitfield assignment is properly + truncated and does not generate a redundant load. */ + +/* Check that we get one load for each simple assign and two for the + compound assign (load the old value before the add then load again + to store back). Also check that our g0 pattern is good. */ +// RUN: clang -O0 -emit-llvm -o %t %s && +// RUN: grep 'load ' %t | count 5 && +// RUN: grep "@g0" %t | count 4 && + +// Check that we got the right value. +// RUN: clang -O3 -emit-llvm -o %t %s && +// RUN: grep 'load ' %t | count 0 && +// RUN: grep "@g0" %t | count 0 + +struct s0 { + int f0 : 2; + _Bool f1 : 1; + unsigned f2 : 2; +}; + +int g0(); + +void f0(void) { + struct s0 s; + if ((s.f0 = 3) != -1) g0(); +} + +void f1(void) { + struct s0 s; + if ((s.f1 = 3) != 1) g0(); +} + +void f2(void) { + struct s0 s; + if ((s.f2 = 3) != 3) g0(); +} + +void f3(void) { + struct s0 s; + // Just check this one for load counts. + s.f0 += 3; +} + diff --git a/test/CodeGen/volatile.c b/test/CodeGen/volatile.c index 4db4a5d843..ed0a33e986 100644 --- a/test/CodeGen/volatile.c +++ b/test/CodeGen/volatile.c @@ -1,4 +1,4 @@ -// RUN: clang -emit-llvm < %s | grep volatile | count 26 +// RUN: clang -emit-llvm < %s | grep volatile | count 25 // The number 26 comes from the current codegen for volatile loads; // if this number changes, it's not necessarily something wrong, but @@ -76,7 +76,7 @@ void main() { vpF2->x=i; vF3.x.y=i; BF.x=i; - vBF.x=i; // FIXME: This generates an extra volatile load + vBF.x=i; V[3]=i; vV[3]=i;