[Clang] Handle target-specific builtins returning aggregates.

[simon_tatham]

Summary

A few of the ARM MVE builtins directly return a structure type. This
causes an assertion failure at code-gen time if you try to assign the
result of the builtin to a variable, because the RValue created in
EmitBuiltinExpr from the llvm::Value produced by codegen is always
made by RValue::get(), which creates a non-aggregate RValue that
will fail an assertion when AggExprEmitter::withReturnValueSlot calls
Src.getAggregatePointer(). A similar failure occurs if you try to use
the struct return value directly to extract one field, e.g.
vld2q(address).val[0].

The existing code-gen tests for those MVE builtins pass the returned
structure type directly to the C return statement, which apparently
managed to avoid that particular code path, so we didn't notice the
crash.

Now EmitBuiltinExpr checks the evaluation kind of the builtin's return
value, and does the necessary handling for aggregate returns. I've added
two extra test cases, both of which crashed before this change.

[rjmccall]

clang/lib/CodeGen/CGBuiltin.cpp

4336

The requirement is that we construct an RValue of the right kind for the expression's result type, so the correct approach here is to switch over getEvaluationKind(E->getType()). This is also a more reliable signal than the presence of the return value slot.

We then need to think about how the return value will be returned to us in each case:

• For scalars, RValue::get(V) is right.

• We can assert for now that there are no target builtins returning a complex type; if we ever need to add this, we'll probably have to break apart an IR struct.

• So that just leaves aggregates. I believe we can't rely on ReturnValue being non-null on entry to this function; for example, IIRC we'll pass down a null slot if the code does an immediate member access into the result, e.g. vld2q(...).val[0]. It looks like the custom logic for VLD24 does not do anything reasonable in this case: it returns a first-class struct, which the rest of IRGen will definitely not expect. Probably the most reasonable thing would be to create a slot if we have an aggregate result and the caller didn't give us a slot (out here in EmitBuiltinExpr), and then we can just require EmitTargetBuiltinExpr to return null or something else that makes it clear that they've done the right thing with the slot that was passed in. You can create a slot with CreateMemTemp.

[simon_tatham]

Thanks for the helpful advice!

Here's a revised version that checks getEvaluationKind. I've also added a test for the vld2q(...).val[0] construction you mentioned: you're right, of course, that that also crashed existing clang, and it still crashed with my previous patch applied. This version seems to work in both cases.

and then we can just require EmitTargetBuiltinExpr to return null or something else that makes it clear that they've done the right thing with the slot that was passed in

The current return value of EmitTargetBuiltinExpr will be the llvm::Value * corresponding to the last of a sequence of store instructions that writes the constructed aggregate into the return value slot. There's no need to actually use that Value for anything in this case, but we can still test that it's non-null, to find out whether EmitTargetBuiltinExpr has successfully recognized a builtin and generated some code, or whether we have to fall through to the ErrorUnsupported.

[rjmccall]

In D72271#1807790, @simon_tatham wrote:

The current return value of EmitTargetBuiltinExpr will be the llvm::Value * corresponding to the last of a sequence of store instructions that writes the constructed aggregate into the return value slot. There's no need to actually use that Value for anything in this case, but we can still test that it's non-null, to find out whether EmitTargetBuiltinExpr has successfully recognized a builtin and generated some code, or whether we have to fall through to the ErrorUnsupported.

Ah, yes, good point.

clang/lib/CodeGen/CGBuiltin.cpp

4354

I would just structure this as:

// 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()) { ... }

// Try to emit a target builtin.
if (Value *V = EmitTargetBuiltinExpr(BuiltinID, E, ReturnValue)) {
 // Turn the result into the appropriate kind of RValue.
 switch (EvalKind) { ... }
}

[simon_tatham]

Restructured as suggested.

[rjmccall]

clang/lib/CodeGen/CGBuiltin.cpp

4356

We generally don't add default cases to exhaustive switches. You can put this llvm_unreachable after the switch.

[simon_tatham]

Moved the llvm_unreachable.

[rjmccall]

Thanks, LGTM!

clang/lib/CodeGen/CGBuiltin.cpp

if (RetTy != V->getType()) {
       assert(V->getType()->canLosslesslyBitCastTo(RetTy) &&
              "Must be able to losslessly bit cast result type");
       V = Builder.CreateBitCast(V, RetTy);
     }
 
     return RValue::get(V);
   }
 
-  // See if we have a target specific builtin that needs to be lowered.
-  if (Value *V = EmitTargetBuiltinExpr(BuiltinID, E, ReturnValue))
+  // 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:
       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");
+  }
 
   ErrorUnsupported(E, "builtin function");
 
   // Unknown builtin, for now just dump it out and return undef.
   return GetUndefRValue(E->getType());
 }
 
 static Value *EmitTargetArchBuiltinExpr(CodeGenFunction *CGF,

clang/test/CodeGen/arm-mve-intrinsics/vld24.c

 void test_vst2q_f16(float16_t *addr, float16x8x2_t value)
 {
 #ifdef POLYMORPHIC
     vst2q(addr, value);
 #else /* POLYMORPHIC */
     vst2q_f16(addr, value);
 #endif /* POLYMORPHIC */
 }
+
+// CHECK-LABEL: @load_into_variable(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[TMP0:%.*]] = call { <8 x i16>, <8 x i16> } @llvm.arm.mve.vld2q.v8i16.p0i16(i16* [[ADDR:%.*]])
+// CHECK-NEXT:    [[TMP1:%.*]] = extractvalue { <8 x i16>, <8 x i16> } [[TMP0]], 0
+// CHECK-NEXT:    [[TMP2:%.*]] = insertvalue [[STRUCT_UINT16X8X2_T:%.*]] undef, <8 x i16> [[TMP1]], 0, 0
+// CHECK-NEXT:    [[TMP3:%.*]] = extractvalue { <8 x i16>, <8 x i16> } [[TMP0]], 1
+// CHECK-NEXT:    [[TMP4:%.*]] = insertvalue [[STRUCT_UINT16X8X2_T]] [[TMP2]], <8 x i16> [[TMP3]], 0, 1
+// CHECK-NEXT:    store <8 x i16> [[TMP1]], <8 x i16>* [[VALUES:%.*]], align 8
+// CHECK-NEXT:    [[ARRAYIDX4:%.*]] = getelementptr inbounds <8 x i16>, <8 x i16>* [[VALUES]], i32 1
+// CHECK-NEXT:    store <8 x i16> [[TMP3]], <8 x i16>* [[ARRAYIDX4]], align 8
+// CHECK-NEXT:    ret void
+//
+void load_into_variable(const uint16_t *addr, uint16x8_t *values)
+{
+    uint16x8x2_t v;
+#ifdef POLYMORPHIC
+    v = vld2q(addr);
+#else /* POLYMORPHIC */
+    v = vld2q_u16(addr);
+#endif /* POLYMORPHIC */
+    values[0] = v.val[0];
+    values[1] = v.val[1];
+}
+
+// CHECK-LABEL: @extract_one_vector(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[TMP0:%.*]] = call { <4 x i32>, <4 x i32> } @llvm.arm.mve.vld2q.v4i32.p0i32(i32* [[ADDR:%.*]])
+// CHECK-NEXT:    [[TMP1:%.*]] = extractvalue { <4 x i32>, <4 x i32> } [[TMP0]], 0
+// CHECK-NEXT:    [[TMP2:%.*]] = insertvalue [[STRUCT_INT32X4X2_T:%.*]] undef, <4 x i32> [[TMP1]], 0, 0
+// CHECK-NEXT:    [[TMP3:%.*]] = extractvalue { <4 x i32>, <4 x i32> } [[TMP0]], 1
+// CHECK-NEXT:    [[TMP4:%.*]] = insertvalue [[STRUCT_INT32X4X2_T]] [[TMP2]], <4 x i32> [[TMP3]], 0, 1
+// CHECK-NEXT:    ret <4 x i32> [[TMP1]]
+//
+int32x4_t extract_one_vector(const int32_t *addr)
+{
+#ifdef POLYMORPHIC
+    return vld2q(addr).val[0];
+#else /* POLYMORPHIC */
+    return vld2q_s32(addr).val[0];
+#endif /* POLYMORPHIC */
+}