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 95733abbab4fe9e4335768107b7069efd5346b3a..9328a535670360e377e0b32dc418a0f4eb761e82 100644 (file)
--- 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 d87edc3e7b2692af9f340959b38a03123a5dd0fb..b236a6ad31ebcbd71f9a7223da5bdbaba3929098 100644 (file)
--- 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 5d99dba69a8cb1ab903fdec5904fcd1351b457f7..05b2793456bc7ac0b5f752148b9cf03b93978c1c 100644 (file)
--- 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 33fbfdb744ad366d21c8c27e6626905103bb6ae2..12a08e161b0513631311ed8c43854138656ceecd 100644 (file)
--- 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 (file)
index 0000000..fc163b4
--- /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 4db4a5d84318bff3aa78857682b2bfe316bbcf49..ed0a33e98628b21e58fb35bf23e1dd61709cf91a 100644 (file)
--- 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;