Add support for ignored bitfield conditional codegen.

[erichkeane]

Summary

Currently we emit an error in just about every case of conditionals
with a 'non simple' branch if treated as an LValue. This patch adds
support for the special case where this is an 'ignored' lvalue, which
permits the side effects from happening.

It also splits up the emit for conditional LValue in a way that should
be usable to handle simple assignment expressions in similar situations.

[efriedma]

This is basically what I expected; the general approach seems fine.

clang/lib/CodeGen/CGExpr.cpp

200

Is there some reason we need to special-case bitfields here?

4635

If we're calling this from EmitIgnoredConditionalOperator, should we be calling back into EmitIgnoredExpr, instead of using EmitLValueOrThrowExpression? Consider, e.g. cond ? s1.field1 = val1 : cond2 ? s1.field2 = val2 : s1.field3 = val3;.

[erichkeane]

clang/lib/CodeGen/CGExpr.cpp

200

I don't think it ends up being a problem to support the 'simple' case (or perhaps even the matrix/vector cases?), but I was not really motivated to write test cases for all of those cases (nor write error handling, since that is already so well handled).

4635

First, this is the 'generic function' that is called by the normal ConditionalOperator handling as well, so there would be a bit of branching/etc perhaps to make this call 'ignored'.

That said, I'm not really sure what the implications of that would be. I don't really see harm in the example you gave though, so I can put a patch together to do that.

[erichkeane]

Added support for chained-conditional operators as requested. All of the variants I could come up with that I was afraid would be breaking ended up having an rvalue conversion (which makes it 'work').

[efriedma]

LGTM

clang/lib/CodeGen/CGExpr.cpp

llvm::Value *CodeGenFunction::EvaluateExprAsBool(const Expr *E) {
   return EmitComplexToScalarConversion(EmitComplexExpr(E), E->getType(), BoolTy,
                                        Loc);
 }
 
 /// EmitIgnoredExpr - Emit code to compute the specified expression,
 /// ignoring the result.
 void CodeGenFunction::EmitIgnoredExpr(const Expr *E) {
   if (E->isPRValue())
-    return (void) EmitAnyExpr(E, AggValueSlot::ignored(), true);
+    return (void)EmitAnyExpr(E, AggValueSlot::ignored(), true);
 
+  // if this is a bitfield-resulting conditional operator, we can special case
+  // emit this. The normal 'EmitLValue' version of this is particularly
+  // difficult to codegen for, since creating a single "LValue" for two
+  // different sized arguments here is not particularly doable.
+  if (const auto *CondOp = dyn_cast<AbstractConditionalOperator>(
+          E->IgnoreParenNoopCasts(getContext()))) {
+    if (CondOp->getObjectKind() == OK_BitField)
+      return EmitIgnoredConditionalOperator(CondOp);
+  }
+
   // Just emit it as an l-value and drop the result.
   EmitLValue(E);
 }
 
 /// EmitAnyExpr - Emit code to compute the specified expression which
 /// can have any type.  The result is returned as an RValue struct.
 /// If this is an aggregate expression, AggSlot indicates where the
 /// result should be returned.
static Optional<LValue> EmitLValueOrThrowExpression(CodeGenFunction &CGF,
   if (auto *ThrowExpr = dyn_cast<CXXThrowExpr>(Operand->IgnoreParens())) {
     CGF.EmitCXXThrowExpr(ThrowExpr, /*KeepInsertionPoint*/false);
     return None;
   }
 
   return CGF.EmitLValue(Operand);
 }
 
-LValue CodeGenFunction::
-EmitConditionalOperatorLValue(const AbstractConditionalOperator *expr) {
-  if (!expr->isGLValue()) {
-    // ?: here should be an aggregate.
-    assert(hasAggregateEvaluationKind(expr->getType()) &&
-           "Unexpected conditional operator!");
-    return EmitAggExprToLValue(expr);
-  }
-
-  OpaqueValueMapping binding(*this, expr);
-
-  const Expr *condExpr = expr->getCond();
+namespace {
+// Handle the case where the condition is a constant evaluatable simple integer,
+// which means we don't have to separately handle the true/false blocks.
+llvm::Optional<LValue> HandleConditionalOperatorLValueSimpleCase(
+    CodeGenFunction &CGF, const AbstractConditionalOperator *E) {
+  const Expr *condExpr = E->getCond();
   bool CondExprBool;
-  if (ConstantFoldsToSimpleInteger(condExpr, CondExprBool)) {
-    const Expr *live = expr->getTrueExpr(), *dead = expr->getFalseExpr();
-    if (!CondExprBool) std::swap(live, dead);
+  if (CGF.ConstantFoldsToSimpleInteger(condExpr, CondExprBool)) {
+    const Expr *Live = E->getTrueExpr(), *Dead = E->getFalseExpr();
+    if (!CondExprBool)
+      std::swap(Live, Dead);
 
-    if (!ContainsLabel(dead)) {
+    if (!CGF.ContainsLabel(Dead)) {
       // If the true case is live, we need to track its region.
       if (CondExprBool)
-        incrementProfileCounter(expr);
+        CGF.incrementProfileCounter(E);
       // If a throw expression we emit it and return an undefined lvalue
       // because it can't be used.
-      if (auto *ThrowExpr = dyn_cast<CXXThrowExpr>(live->IgnoreParens())) {
-        EmitCXXThrowExpr(ThrowExpr);
-        llvm::Type *ElemTy = ConvertType(dead->getType());
+      if (auto *ThrowExpr = dyn_cast<CXXThrowExpr>(Live->IgnoreParens())) {
+        CGF.EmitCXXThrowExpr(ThrowExpr);
+        llvm::Type *ElemTy = CGF.ConvertType(Dead->getType());
         llvm::Type *Ty = llvm::PointerType::getUnqual(ElemTy);
-        return MakeAddrLValue(
+        return CGF.MakeAddrLValue(
             Address(llvm::UndefValue::get(Ty), ElemTy, CharUnits::One()),
-            dead->getType());
+            Dead->getType());
       }
-      return EmitLValue(live);
+      return CGF.EmitLValue(Live);
     }
   }
+  return llvm::None;
+}
+struct ConditionalInfo {
+  llvm::BasicBlock *lhsBlock, *rhsBlock;
+  Optional<LValue> LHS, RHS;
+};
 
-  llvm::BasicBlock *lhsBlock = createBasicBlock("cond.true");
-  llvm::BasicBlock *rhsBlock = createBasicBlock("cond.false");
-  llvm::BasicBlock *contBlock = createBasicBlock("cond.end");
+// Create and generate the 3 blocks for a conditional operator.
+// Leaves the 'current block' in the continuation basic block.
+ConditionalInfo EmitConditionalBlocks(CodeGenFunction &CGF,
+                                      const AbstractConditionalOperator *E) {
+  ConditionalInfo Info{CGF.createBasicBlock("cond.true"),
+                       CGF.createBasicBlock("cond.false")};
+  llvm::BasicBlock *endBlock = CGF.createBasicBlock("cond.end");
+
+  CodeGenFunction::ConditionalEvaluation eval(CGF);
+  CGF.EmitBranchOnBoolExpr(E->getCond(), Info.lhsBlock, Info.rhsBlock,
+                           CGF.getProfileCount(E));
+
+  // Any temporaries created here are conditional.
+  CGF.EmitBlock(Info.lhsBlock);
+  CGF.incrementProfileCounter(E);
+  eval.begin(CGF);
+  Info.LHS = EmitLValueOrThrowExpression(CGF, E->getTrueExpr());
+  eval.end(CGF);
+  Info.lhsBlock = CGF.Builder.GetInsertBlock();
 
-  ConditionalEvaluation eval(*this);
-  EmitBranchOnBoolExpr(condExpr, lhsBlock, rhsBlock, getProfileCount(expr));
+  if (Info.LHS)
+    CGF.Builder.CreateBr(endBlock);
 
   // Any temporaries created here are conditional.
-  EmitBlock(lhsBlock);
-  incrementProfileCounter(expr);
-  eval.begin(*this);
-  Optional<LValue> lhs =
-      EmitLValueOrThrowExpression(*this, expr->getTrueExpr());
-  eval.end(*this);
+  CGF.EmitBlock(Info.rhsBlock);
+  eval.begin(CGF);
+  Info.RHS = EmitLValueOrThrowExpression(CGF, E->getFalseExpr());
+  eval.end(CGF);
+  Info.rhsBlock = CGF.Builder.GetInsertBlock();
+  CGF.EmitBlock(endBlock);
+
+  return Info;
+}
+} // namespace
+
+void CodeGenFunction::EmitIgnoredConditionalOperator(
+    const AbstractConditionalOperator *E) {
+  if (!E->isGLValue()) {
+    // ?: here should be an aggregate.
+    assert(hasAggregateEvaluationKind(E->getType()) &&
+           "Unexpected conditional operator!");
+    return (void)EmitAggExprToLValue(E);
+  }
 
-  if (lhs && !lhs->isSimple())
-    return EmitUnsupportedLValue(expr, "conditional operator");
+  OpaqueValueMapping binding(*this, E);
+  if (HandleConditionalOperatorLValueSimpleCase(*this, E))
+    return;
 
-  lhsBlock = Builder.GetInsertBlock();
-  if (lhs)
-    Builder.CreateBr(contBlock);
+  EmitConditionalBlocks(*this, E);
+}
+LValue CodeGenFunction::EmitConditionalOperatorLValue(
+    const AbstractConditionalOperator *expr) {
+  if (!expr->isGLValue()) {
+    // ?: here should be an aggregate.
+    assert(hasAggregateEvaluationKind(expr->getType()) &&
+           "Unexpected conditional operator!");
+    return EmitAggExprToLValue(expr);
+  }
 
-  // Any temporaries created here are conditional.
-  EmitBlock(rhsBlock);
-  eval.begin(*this);
-  Optional<LValue> rhs =
-      EmitLValueOrThrowExpression(*this, expr->getFalseExpr());
-  eval.end(*this);
-  if (rhs && !rhs->isSimple())
-    return EmitUnsupportedLValue(expr, "conditional operator");
-  rhsBlock = Builder.GetInsertBlock();
+  OpaqueValueMapping binding(*this, expr);
+  if (llvm::Optional<LValue> Res =
+          HandleConditionalOperatorLValueSimpleCase(*this, expr))
+    return *Res;
 
-  EmitBlock(contBlock);
+  ConditionalInfo Info = EmitConditionalBlocks(*this, expr);
+
+  if ((Info.LHS && !Info.LHS->isSimple()) ||
+      (Info.RHS && !Info.RHS->isSimple()))
+    return EmitUnsupportedLValue(expr, "conditional operator");
 
-  if (lhs && rhs) {
-    Address lhsAddr = lhs->getAddress(*this);
-    Address rhsAddr = rhs->getAddress(*this);
+  if (Info.LHS && Info.RHS) {
+    Address lhsAddr = Info.LHS->getAddress(*this);
+    Address rhsAddr = Info.RHS->getAddress(*this);
     llvm::PHINode *phi = Builder.CreatePHI(lhsAddr.getType(), 2, "cond-lvalue");
-    phi->addIncoming(lhsAddr.getPointer(), lhsBlock);
-    phi->addIncoming(rhsAddr.getPointer(), rhsBlock);
+    phi->addIncoming(lhsAddr.getPointer(), Info.lhsBlock);
+    phi->addIncoming(rhsAddr.getPointer(), Info.rhsBlock);
     Address result(phi, lhsAddr.getElementType(),
                    std::min(lhsAddr.getAlignment(), rhsAddr.getAlignment()));
     AlignmentSource alignSource =
-      std::max(lhs->getBaseInfo().getAlignmentSource(),
-               rhs->getBaseInfo().getAlignmentSource());
+        std::max(Info.LHS->getBaseInfo().getAlignmentSource(),
+                 Info.RHS->getBaseInfo().getAlignmentSource());
     TBAAAccessInfo TBAAInfo = CGM.mergeTBAAInfoForConditionalOperator(
-        lhs->getTBAAInfo(), rhs->getTBAAInfo());
+        Info.LHS->getTBAAInfo(), Info.RHS->getTBAAInfo());
     return MakeAddrLValue(result, expr->getType(), LValueBaseInfo(alignSource),
                           TBAAInfo);
   } else {
-    assert((lhs || rhs) &&
+    assert((Info.LHS || Info.RHS) &&
            "both operands of glvalue conditional are throw-expressions?");
-    return lhs ? *lhs : *rhs;
+    return Info.LHS ? *Info.LHS : *Info.RHS;
   }
 }
 
 /// EmitCastLValue - Casts are never lvalues unless that cast is to a reference
 /// type. If the cast is to a reference, we can have the usual lvalue result,
 /// otherwise if a cast is needed by the code generator in an lvalue context,
 /// then it must mean that we need the address of an aggregate in order to
 /// access one of its members.  This can happen for all the reasons that casts

clang/lib/CodeGen/CodeGenFunction.h

public:
   LValue EmitMatrixSubscriptExpr(const MatrixSubscriptExpr *E);
   LValue EmitOMPArraySectionExpr(const OMPArraySectionExpr *E,
                                  bool IsLowerBound = true);
   LValue EmitExtVectorElementExpr(const ExtVectorElementExpr *E);
   LValue EmitMemberExpr(const MemberExpr *E);
   LValue EmitObjCIsaExpr(const ObjCIsaExpr *E);
   LValue EmitCompoundLiteralLValue(const CompoundLiteralExpr *E);
   LValue EmitInitListLValue(const InitListExpr *E);
+  void EmitIgnoredConditionalOperator(const AbstractConditionalOperator *E);
   LValue EmitConditionalOperatorLValue(const AbstractConditionalOperator *E);
   LValue EmitCastLValue(const CastExpr *E);
   LValue EmitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *E);
   LValue EmitOpaqueValueLValue(const OpaqueValueExpr *e);
 
   Address EmitExtVectorElementLValue(LValue V);
 
   RValue EmitRValueForField(LValue LV, const FieldDecl *FD, SourceLocation Loc);

clang/test/CodeGenCXX/ignored-bitfield-conditional.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+struct S {
+  int field1 : 5;
+  int field2 : 6;
+};
+
+void use(bool cond, struct S s1, struct S s2, int val1, int val2) {
+  // CHECK: define {{.*}}use{{.*}}(
+  // CHECK: %[[S1:.+]] = alloca %struct.S
+  // CHECK: %[[S2:.+]] = alloca %struct.S
+  // CHECK: %[[COND:.+]] = alloca i8
+  // CHECK: %[[VAL1:.+]] = alloca i32
+  // CHECK: %[[VAL2:.+]] = alloca i32
+
+  cond ? s1.field1 = val1 : s1.field2 = val2;
+  // Condition setup, branch.
+  // CHECK: %[[CONDLD:.+]] = load i8, ptr %[[COND]]
+  // CHECK: %[[TO_BOOL:.+]] = trunc i8 %[[CONDLD]] to i1
+  // CHECK: br i1 %[[TO_BOOL]], label %[[TRUE:.+]], label %[[FALSE:.+]]
+
+  // 'True', branch set the BF, branch to 'end'.
+  // CHECK: [[TRUE]]:
+  // CHECK: %[[VAL1LD:.+]] = load i32, ptr %[[VAL1]]
+  // CHECK: %[[VAL1TRUNC:.+]] = trunc i32 %[[VAL1LD]] to i16
+  // CHECK: %[[BF_LOAD:.+]] = load i16, ptr %[[S1]]
+  // CHECK: %[[BF_VAL:.+]] = and i16 %[[VAL1TRUNC]], 31
+  // CHECK: %[[BF_CLEAR:.+]] = and i16 %[[BF_LOAD]], -32
+  // CHECK: %[[BF_SET:.+]] = or i16 %[[BF_CLEAR]], %[[BF_VAL]]
+  // CHECK: store i16 %[[BF_SET]], ptr %[[S1]]
+  // CHECK: br label %[[END:.+]]
+
+  // 'False', branch set the OTHER BF, branch to 'end'.
+  // CHECK: [[FALSE]]:
+  // CHECK: %[[VAL2LD:.+]] = load i32, ptr %[[VAL2]]
+  // CHECK: %[[VAL2TRUNC:.+]] = trunc i32 %[[VAL2LD]] to i16
+  // CHECK: %[[BF_LOAD:.+]] = load i16, ptr %[[S1]]
+  // CHECK: %[[BF_VAL:.+]] = and i16 %[[VAL2TRUNC]], 63 
+  // CHECK: %[[BF_SHIFT:.+]] = shl i16 %[[BF_VAL]], 5
+  // CHECK: %[[BF_CLEAR:.+]] = and i16 %[[BF_LOAD]], -2017
+  // CHECK: %[[BF_SET:.+]] = or i16 %[[BF_CLEAR]], %[[BF_SHIFT]]
+  // CHECK: store i16 %[[BF_SET]], ptr %[[S1]]
+  // CHECK: br label %[[END:.+]]
+
+  // CHECK: [[END]]:
+  // There is nothing in the 'end' block associated with this, but it is the
+  // 'continuation' block for the rest of the function.
+
+  // Same test, has a no-op cast and parens.
+  (int)(cond ? s2.field1 = val1 : s2.field2 = val2);
+  // Condition setup, branch.
+  // CHECK: %[[CONDLD:.+]] = load i8, ptr %[[COND]]
+  // CHECK: %[[TO_BOOL:.+]] = trunc i8 %[[CONDLD]] to i1
+  // CHECK: br i1 %[[TO_BOOL]], label %[[TRUE:.+]], label %[[FALSE:.+]]
+
+  // 'True', branch set the BF, branch to 'end'.
+  // CHECK: [[TRUE]]:
+  // CHECK: %[[VAL1LD:.+]] = load i32, ptr %[[VAL1]]
+  // CHECK: %[[VAL1TRUNC:.+]] = trunc i32 %[[VAL1LD]] to i16
+  // CHECK: %[[BF_LOAD:.+]] = load i16, ptr %[[S2]]
+  // CHECK: %[[BF_VAL:.+]] = and i16 %[[VAL1TRUNC]], 31
+  // CHECK: %[[BF_CLEAR:.+]] = and i16 %[[BF_LOAD]], -32
+  // CHECK: %[[BF_SET:.+]] = or i16 %[[BF_CLEAR]], %[[BF_VAL]]
+  // CHECK: store i16 %[[BF_SET]], ptr %[[S2]]
+  // CHECK: br label %[[END:.+]]
+
+  // 'False', branch set the OTHER BF, branch to 'end'.
+  // CHECK: [[FALSE]]:
+  // CHECK: %[[VAL2LD:.+]] = load i32, ptr %[[VAL2]]
+  // CHECK: %[[VAL2TRUNC:.+]] = trunc i32 %[[VAL2LD]] to i16
+  // CHECK: %[[BF_LOAD:.+]] = load i16, ptr %[[S2]]
+  // CHECK: %[[BF_VAL:.+]] = and i16 %[[VAL2TRUNC]], 63 
+  // CHECK: %[[BF_SHIFT:.+]] = shl i16 %[[BF_VAL]], 5
+  // CHECK: %[[BF_CLEAR:.+]] = and i16 %[[BF_LOAD]], -2017
+  // CHECK: %[[BF_SET:.+]] = or i16 %[[BF_CLEAR]], %[[BF_SHIFT]]
+  // CHECK: store i16 %[[BF_SET]], ptr %[[S2]]
+  // CHECK: br label %[[END:.+]]
+
+  // CHECK: [[END]]:
+  // There is nothing in the 'end' block associated with this, but it is the
+  // 'continuation' block for the rest of the function.
+
+}