[clang][CodeGen] Consistently return nullptr Values for void builtins and scalar initalization

[zero9178]

Summary

A common post condition of the various visitor functions in CodeGen is that instructions, that do not return any values, simply return a nullptr Value as a sentinel. This has not been the case however for calls to some builtins returning void, as well as for an initializer expression of the form void(). This would then lead to ICEs in CodeGen on code relying on nullptr being returned for void values, which is eg. the case for conditional expressions [0].
This patch fixes that by returning nullptr Values for intrinsics known not to return any values as well as for a scalar initializer returning void.

Fixes https://github.com/llvm/llvm-project/issues/53127

[0] https://github.com/llvm/llvm-project/blob/266ec801fb23f9f5f1d61ca9466e0805fbdb78a7/clang/lib/CodeGen/CGExprScalar.cpp#L4849-L4892

---

Note:
I opted to adjust these functions as an idealistic goal to be more consistent with the rest of the codebase. An alternative implementation that would have also fixed the above issue and would be less code is to simply handle it within the visit function of the conditional expression, by simply checking if it the type of the expression is void and returning bailing out by returning nullptr there instead of creating the invalid phi node. If you prefer that version or something of the sorts please let me know

[zero9178]

Rebase and boop CI

[zero9178]

clang-format and specify target triple in test since mangling is target dependent

[efriedma]

LGTM

clang/lib/CodeGen/CGBuiltin.cpp

RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
   default: break;
   case Builtin::BI__builtin___CFStringMakeConstantString:
   case Builtin::BI__builtin___NSStringMakeConstantString:
     return RValue::get(ConstantEmitter(*this).emitAbstract(E, E->getType()));
   case Builtin::BI__builtin_stdarg_start:
   case Builtin::BI__builtin_va_start:
   case Builtin::BI__va_start:
   case Builtin::BI__builtin_va_end:
-    return RValue::get(
-        EmitVAStartEnd(BuiltinID == Builtin::BI__va_start
-                           ? EmitScalarExpr(E->getArg(0))
-                           : EmitVAListRef(E->getArg(0)).getPointer(),
-                       BuiltinID != Builtin::BI__builtin_va_end));
+                   BuiltinID != Builtin::BI__builtin_va_end);
+    return RValue::get(nullptr);
   case Builtin::BI__builtin_va_copy: {
     Value *DstPtr = EmitVAListRef(E->getArg(0)).getPointer();
     Value *SrcPtr = EmitVAListRef(E->getArg(1)).getPointer();
 
     llvm::Type *Type = Int8PtrTy;
 
     DstPtr = Builder.CreateBitCast(DstPtr, Type);
     SrcPtr = Builder.CreateBitCast(SrcPtr, Type);
-    return RValue::get(Builder.CreateCall(CGM.getIntrinsic(Intrinsic::vacopy),
-                                          {DstPtr, SrcPtr}));
+    Builder.CreateCall(CGM.getIntrinsic(Intrinsic::vacopy),
+                       {DstPtr, SrcPtr});
+    return RValue::get(nullptr);
   }
   case Builtin::BI__builtin_abs:
   case Builtin::BI__builtin_labs:
   case Builtin::BI__builtin_llabs: {
     // X < 0 ? -X : X
     // The negation has 'nsw' because abs of INT_MIN is undefined.
     Value *ArgValue = EmitScalarExpr(E->getArg(0));
     Value *NegOp = Builder.CreateNSWNeg(ArgValue, "neg");
RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
   }
   case Builtin::BI__assume:
   case Builtin::BI__builtin_assume: {
     if (E->getArg(0)->HasSideEffects(getContext()))
       return RValue::get(nullptr);
 
     Value *ArgValue = EmitScalarExpr(E->getArg(0));
     Function *FnAssume = CGM.getIntrinsic(Intrinsic::assume);
-    return RValue::get(Builder.CreateCall(FnAssume, ArgValue));
+    Builder.CreateCall(FnAssume, ArgValue);
+    return RValue::get(nullptr);
   }
   case Builtin::BI__arithmetic_fence: {
     // Create the builtin call if FastMath is selected, and the target
     // supports the builtin, otherwise just return the argument.
     CodeGenFunction::CGFPOptionsRAII FPOptsRAII(*this, E);
     llvm::FastMathFlags FMF = Builder.getFastMathFlags();
     bool isArithmeticFenceEnabled =
         FMF.allowReassoc() &&
case Builtin::BI__builtin_prefetch: {
     Value *Locality, *RW, *Address = EmitScalarExpr(E->getArg(0));
     // FIXME: Technically these constants should of type 'int', yes?
     RW = (E->getNumArgs() > 1) ? EmitScalarExpr(E->getArg(1)) :
       llvm::ConstantInt::get(Int32Ty, 0);
     Locality = (E->getNumArgs() > 2) ? EmitScalarExpr(E->getArg(2)) :
       llvm::ConstantInt::get(Int32Ty, 3);
     Value *Data = llvm::ConstantInt::get(Int32Ty, 1);
     Function *F = CGM.getIntrinsic(Intrinsic::prefetch, Address->getType());
-    return RValue::get(Builder.CreateCall(F, {Address, RW, Locality, Data}));
+    Builder.CreateCall(F, {Address, RW, Locality, Data});
+    return RValue::get(nullptr);
   }
   case Builtin::BI__builtin_readcyclecounter: {
     Function *F = CGM.getIntrinsic(Intrinsic::readcyclecounter);
     return RValue::get(Builder.CreateCall(F));
   }
   case Builtin::BI__builtin___clear_cache: {
     Value *Begin = EmitScalarExpr(E->getArg(0));
     Value *End = EmitScalarExpr(E->getArg(1));
     Function *F = CGM.getIntrinsic(Intrinsic::clear_cache);
     return RValue::get(Builder.CreateCall(F, {Begin, End}));
   }
   case Builtin::BI__builtin_trap:
-    return RValue::get(EmitTrapCall(Intrinsic::trap));
+    EmitTrapCall(Intrinsic::trap);
+    return RValue::get(nullptr);
   case Builtin::BI__debugbreak:
-    return RValue::get(EmitTrapCall(Intrinsic::debugtrap));
+    EmitTrapCall(Intrinsic::debugtrap);
+    return RValue::get(nullptr);
   case Builtin::BI__builtin_unreachable: {
     EmitUnreachable(E->getExprLoc());
 
     // We do need to preserve an insertion point.
     EmitBlock(createBasicBlock("unreachable.cont"));
 
     return RValue::get(nullptr);
   }
case Builtin::BI__builtin_eh_return: {
 
     // We do need to preserve an insertion point.
     EmitBlock(createBasicBlock("builtin_eh_return.cont"));
 
     return RValue::get(nullptr);
   }
   case Builtin::BI__builtin_unwind_init: {
     Function *F = CGM.getIntrinsic(Intrinsic::eh_unwind_init);
-    return RValue::get(Builder.CreateCall(F));
+    Builder.CreateCall(F);
+    return RValue::get(nullptr);
   }
   case Builtin::BI__builtin_extend_pointer: {
     // Extends a pointer to the size of an _Unwind_Word, which is
     // uint64_t on all platforms.  Generally this gets poked into a
     // register and eventually used as an address, so if the
     // addressing registers are wider than pointers and the platform
     // doesn't implicitly ignore high-order bits when doing
     // addressing, we need to make sure we zext / sext based on
case Builtin::BI__builtin_addressof:
     return RValue::get(EmitLValue(E->getArg(0)).getPointer(*this));
   case Builtin::BI__builtin_function_start:
     return RValue::get(CGM.GetFunctionStart(
         E->getArg(0)->getAsBuiltinConstantDeclRef(CGM.getContext())));
   case Builtin::BI__builtin_operator_new:
     return EmitBuiltinNewDeleteCall(
         E->getCallee()->getType()->castAs<FunctionProtoType>(), E, false);
   case Builtin::BI__builtin_operator_delete:
-    return EmitBuiltinNewDeleteCall(
+    EmitBuiltinNewDeleteCall(
         E->getCallee()->getType()->castAs<FunctionProtoType>(), E, true);
+    return RValue::get(nullptr);
 
   case Builtin::BI__builtin_is_aligned:
     return EmitBuiltinIsAligned(E);
   case Builtin::BI__builtin_align_up:
     return EmitBuiltinAlignTo(E, true);
   case Builtin::BI__builtin_align_down:
     return EmitBuiltinAlignTo(E, false);
 

   case Builtin::BI__fastfail:
     return RValue::get(EmitMSVCBuiltinExpr(MSVCIntrin::__fastfail, E));
 
   case Builtin::BI__builtin_coro_id:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_id);
   case Builtin::BI__builtin_coro_promise:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_promise);
   case Builtin::BI__builtin_coro_resume:
-    return EmitCoroutineIntrinsic(E, Intrinsic::coro_resume);
+    EmitCoroutineIntrinsic(E, Intrinsic::coro_resume);
+    return RValue::get(nullptr);
   case Builtin::BI__builtin_coro_frame:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_frame);
   case Builtin::BI__builtin_coro_noop:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_noop);
   case Builtin::BI__builtin_coro_free:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_free);
   case Builtin::BI__builtin_coro_destroy:
-    return EmitCoroutineIntrinsic(E, Intrinsic::coro_destroy);
+    EmitCoroutineIntrinsic(E, Intrinsic::coro_destroy);
+    return RValue::get(nullptr);
   case Builtin::BI__builtin_coro_done:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_done);
   case Builtin::BI__builtin_coro_alloc:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_alloc);
   case Builtin::BI__builtin_coro_begin:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_begin);
   case Builtin::BI__builtin_coro_end:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_end);
return RValue::get(EmitRuntimeCall(
         {NDRange, Kernel, Block}));
   }
 
   case Builtin::BI__builtin_store_half:
   case Builtin::BI__builtin_store_halff: {
     Value *Val = EmitScalarExpr(E->getArg(0));
     Address Address = EmitPointerWithAlignment(E->getArg(1));
     Value *HalfVal = Builder.CreateFPTrunc(Val, Builder.getHalfTy());
-    return RValue::get(Builder.CreateStore(HalfVal, Address));
+    Builder.CreateStore(HalfVal, Address);
+    return RValue::get(nullptr);
   }
   case Builtin::BI__builtin_load_half: {
     Address Address = EmitPointerWithAlignment(E->getArg(0));
     Value *HalfVal = Builder.CreateLoad(Address);
     return RValue::get(Builder.CreateFPExt(HalfVal, Builder.getDoubleTy()));
   }
   case Builtin::BI__builtin_load_halff: {
     Address Address = EmitPointerWithAlignment(E->getArg(0));
if (RetTy != V->getType()) {
       // in amx intrinsics.
       if (V->getType()->isX86_AMXTy())
         V = Builder.CreateIntrinsic(Intrinsic::x86_cast_tile_to_vector, {RetTy},
                                     {V});
       else
         V = Builder.CreateBitCast(V, RetTy);
     }
 
+    if (RetTy->isVoidTy())
+      return RValue::get(nullptr);
+
     return RValue::get(V);
   }
 
   // Some target-specific builtins can have aggregate return values, e.g.
   // __builtin_arm_mve_vld2q_u32. So if the result is an aggregate, force
   // ReturnValue to be non-null, so that the target-specific emission code can
   // always just emit into it.
   TypeEvaluationKind EvalKind = getEvaluationKind(E->getType());
   if (EvalKind == TEK_Aggregate && ReturnValue.isNull()) {
     Address DestPtr = CreateMemTemp(E->getType(), "agg.tmp");
     ReturnValue = ReturnValueSlot(DestPtr, false);
   }
 
   // Now see if we can emit a target-specific builtin.
   if (Value *V = EmitTargetBuiltinExpr(BuiltinID, E, ReturnValue)) {
     switch (EvalKind) {
     case TEK_Scalar:
+      if (V->getType()->isVoidTy())
+        return RValue::get(nullptr);
       return RValue::get(V);
     case TEK_Aggregate:
       return RValue::getAggregate(ReturnValue.getValue(),
                                   ReturnValue.isVolatile());
     case TEK_Complex:
       llvm_unreachable("No current target builtin returns complex");
     }
     llvm_unreachable("Bad evaluation kind in EmitBuiltinExpr");

clang/lib/CodeGen/CGExprScalar.cpp

public:
   }
   Value *VisitObjCBoolLiteralExpr(const ObjCBoolLiteralExpr *E) {
     return llvm::ConstantInt::get(ConvertType(E->getType()), E->getValue());
   }
   Value *VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *E) {
     return llvm::ConstantInt::get(ConvertType(E->getType()), E->getValue());
   }
   Value *VisitCXXScalarValueInitExpr(const CXXScalarValueInitExpr *E) {
+    if (E->getType()->isVoidType())
+      return nullptr;
+
     return EmitNullValue(E->getType());
   }
   Value *VisitGNUNullExpr(const GNUNullExpr *E) {
     return EmitNullValue(E->getType());
   }
   Value *VisitOffsetOfExpr(OffsetOfExpr *E);
   Value *VisitUnaryExprOrTypeTraitExpr(const UnaryExprOrTypeTraitExpr *E);
   Value *VisitAddrLabelExpr(const AddrLabelExpr *E) {

clang/test/CodeGen/pr53127.cpp

+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+
+bool e();
+
+void operator delete(void*);
+
+// CHECK-LABEL: @_Z1fPiz(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[P_ADDR:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:    [[L:%.*]] = alloca [1 x %struct.__va_list_tag], align 16
+// CHECK-NEXT:    [[L2:%.*]] = alloca [1 x %struct.__va_list_tag], align 16
+// CHECK-NEXT:    store ptr [[P:%.*]], ptr [[P_ADDR]], align 8
+// CHECK-NEXT:    [[CALL:%.*]] = call noundef zeroext i1 @_Z1ev()
+// CHECK-NEXT:    br i1 [[CALL]], label [[COND_TRUE:%.*]], label [[COND_FALSE:%.*]]
+// CHECK:       cond.true:
+// CHECK-NEXT:    call void @llvm.trap()
+// CHECK-NEXT:    br label [[COND_END:%.*]]
+// CHECK:       cond.false:
+// CHECK-NEXT:    br label [[COND_END]]
+// CHECK:       cond.end:
+// CHECK-NEXT:    [[CALL1:%.*]] = call noundef zeroext i1 @_Z1ev()
+// CHECK-NEXT:    br i1 [[CALL1]], label [[COND_TRUE2:%.*]], label [[COND_FALSE3:%.*]]
+// CHECK:       cond.true2:
+// CHECK-NEXT:    call void @llvm.debugtrap()
+// CHECK-NEXT:    br label [[COND_END4:%.*]]
+// CHECK:       cond.false3:
+// CHECK-NEXT:    br label [[COND_END4]]
+// CHECK:       cond.end4:
+// CHECK-NEXT:    [[CALL5:%.*]] = call noundef zeroext i1 @_Z1ev()
+// CHECK-NEXT:    [[TMP0:%.*]] = zext i1 [[CALL5]] to i64
+// CHECK-NEXT:    call void @llvm.assume(i1 true)
+// CHECK-NEXT:    [[CALL6:%.*]] = call noundef zeroext i1 @_Z1ev()
+// CHECK-NEXT:    br i1 [[CALL6]], label [[COND_TRUE7:%.*]], label [[COND_FALSE8:%.*]]
+// CHECK:       cond.true7:
+// CHECK-NEXT:    [[ARRAYDECAY:%.*]] = getelementptr inbounds [1 x %struct.__va_list_tag], ptr [[L]], i64 0, i64 0
+// CHECK-NEXT:    call void @llvm.va_start(ptr [[ARRAYDECAY]])
+// CHECK-NEXT:    br label [[COND_END9:%.*]]
+// CHECK:       cond.false8:
+// CHECK-NEXT:    br label [[COND_END9]]
+// CHECK:       cond.end9:
+// CHECK-NEXT:    [[CALL10:%.*]] = call noundef zeroext i1 @_Z1ev()
+// CHECK-NEXT:    br i1 [[CALL10]], label [[COND_TRUE11:%.*]], label [[COND_FALSE14:%.*]]
+// CHECK:       cond.true11:
+// CHECK-NEXT:    [[ARRAYDECAY12:%.*]] = getelementptr inbounds [1 x %struct.__va_list_tag], ptr [[L]], i64 0, i64 0
+// CHECK-NEXT:    [[ARRAYDECAY13:%.*]] = getelementptr inbounds [1 x %struct.__va_list_tag], ptr [[L2]], i64 0, i64 0
+// CHECK-NEXT:    call void @llvm.va_copy(ptr [[ARRAYDECAY12]], ptr [[ARRAYDECAY13]])
+// CHECK-NEXT:    br label [[COND_END15:%.*]]
+// CHECK:       cond.false14:
+// CHECK-NEXT:    br label [[COND_END15]]
+// CHECK:       cond.end15:
+// CHECK-NEXT:    [[CALL16:%.*]] = call noundef zeroext i1 @_Z1ev()
+// CHECK-NEXT:    br i1 [[CALL16]], label [[COND_TRUE17:%.*]], label [[COND_FALSE18:%.*]]
+// CHECK:       cond.true17:
+// CHECK-NEXT:    [[TMP1:%.*]] = load ptr, ptr [[P_ADDR]], align 8
+// CHECK-NEXT:    call void @llvm.prefetch.p0(ptr [[TMP1]], i32 0, i32 3, i32 1)
+// CHECK-NEXT:    br label [[COND_END19:%.*]]
+// CHECK:       cond.false18:
+// CHECK-NEXT:    br label [[COND_END19]]
+// CHECK:       cond.end19:
+// CHECK-NEXT:    [[CALL20:%.*]] = call noundef zeroext i1 @_Z1ev()
+// CHECK-NEXT:    br i1 [[CALL20]], label [[COND_TRUE21:%.*]], label [[COND_FALSE22:%.*]]
+// CHECK:       cond.true21:
+// CHECK-NEXT:    call void @llvm.eh.unwind.init()
+// CHECK-NEXT:    br label [[COND_END23:%.*]]
+// CHECK:       cond.false22:
+// CHECK-NEXT:    br label [[COND_END23]]
+// CHECK:       cond.end23:
+// CHECK-NEXT:    [[CALL24:%.*]] = call noundef zeroext i1 @_Z1ev()
+// CHECK-NEXT:    [[TMP2:%.*]] = zext i1 [[CALL24]] to i64
+// CHECK-NEXT:    [[TMP3:%.*]] = load ptr, ptr [[P_ADDR]], align 8
+// CHECK-NEXT:    call void @_ZdlPv(ptr noundef [[TMP3]]) #[[ATTR8:[0-9]+]]
+// CHECK-NEXT:    ret void
+//
+void f(int* p, ...)
+{
+  e() ? __builtin_trap() : void();
+  e() ? __builtin_debugtrap() : void();
+  e() ? __builtin_assume(true) : void();
+  __builtin_va_list l;
+  e() ? __builtin_va_start(l, p) : void();
+  __builtin_va_list l2;
+  e() ? __builtin_va_copy(l, l2) : void();
+  e() ? __builtin_prefetch(p) : void();
+  e() ? __builtin_unwind_init() : void();
+  e() ? __builtin_operator_delete(p) : void();
+}