[CodeGen] Fix crash when a function taking transparent union is redeclared.

[vsapsai]

Summary

When a function taking transparent union is declared as taking one of
union members earlier in the translation unit, clang would hit an
"Invalid cast" assertion during EmitFunctionProlog. This case
corresponds to function f1 in test/CodeGen/transparent-union-redecl.c.
We decided to cast i32 to union because after merging function
declarations function parameter type becomes int,
CGFunctionInfo::ArgInfo type matches with ABIArgInfo type, so we decide
it is a trivial case. But these types should also be castable to
parameter declaration type which is not the case here.

The fix is in checking for the trivial case if we would be able to cast
to parameter declaration type. It exposed inconsistency that we check
hasScalarEvaluationKind for different types in EmitParmDecl and
EmitFunctionProlog, and comment says they should match.

Additional tests in Sema/transparent-union.c capture current behavior.
Added them because when trying different fix I regressed on those cases
but there were no tests to catch it.

rdar://problem/34949329

[rjmccall]

clang/lib/CodeGen/CGCall.cpp

2321

I think the right fix is to change the second clause to ArgI.getCoerceToType() == ConvertType(Arg->getType()). The point of this special case is to catch the common case that the natural way that IRGen wants to emit the argument expression is by producing a single scalar of exactly the right IR type; it seems to me that that condition would capture that correctly.

[vsapsai]

clang/lib/CodeGen/CGCall.cpp

2321

I've tried that and it almost works. There are 4 failing tests

Clang :: CodeGen/kr-func-promote.c
Clang :: CodeGenCXX/microsoft-abi-multiple-nonvirtual-inheritance.cpp
Clang :: CodeGenCXX/microsoft-abi-virtual-inheritance-vtordisps.cpp
Clang :: CodeGenCXX/microsoft-abi-virtual-inheritance.cpp

In a couple of places the problem is that expected function i32 @a(i32) but received i32 @a(i32 %x.coerce).

As for me, parameter naming is not really a problem and can be fixed by making tests less specific. What is more interesting, with the change we started adding extra store and load. For example, for CodeGen/kr-func-promote.c IR was

define i32 @a(i32) #0 {
  %x.addr = alloca i16, align 2
  %x = trunc i32 %0 to i16
  store i16 %x, i16* %x.addr, align 2
  %2 = load i16, i16* %x.addr, align 2
  %conv = sext i16 %2 to i32
  ret i32 %conv
}

and with type comparison change IR becomes

define i32 @a(i32 %x.coerce) #0 {
entry:
  %x = alloca i16, align 2
  %x.addr = alloca i16, align 2
  %coerce.val.ii = trunc i32 %x.coerce to i16
  store i16 %coerce.val.ii, i16* %x, align 2
  %x1 = load i16, i16* %x, align 2
  store i16 %x1, i16* %x.addr, align 2
  %0 = load i16, i16* %x.addr, align 2
  %conv = sext i16 %0 to i32
  ret i32 %conv
}

The same situation is with "microsoft-abi-*" tests where instead of

%this = bitcast i8* %0 to %struct.D*
store %struct.D* %this, %struct.D** %this.addr, align 4
%this1 = load %struct.D*, %struct.D** %this.addr, align 4
%2 = bitcast %struct.D* %this1 to i8*

we have

%coerce.val = bitcast i8* %this.coerce to %struct.D*
store %struct.D* %coerce.val, %struct.D** %this, align 4
%this1 = load %struct.D*, %struct.D** %this, align 4
store %struct.D* %this1, %struct.D** %this.addr, align 4
%this2 = load %struct.D*, %struct.D** %this.addr, align 4
%0 = bitcast %struct.D* %this2 to i8*

I'll check where and why we are adding extra store/load.

[vsapsai]

clang/lib/CodeGen/CGCall.cpp

2321

The last store comes from CodeGenFunction::EmitParmDecl where for direct arguments we have

if (Arg.isIndirect()) {
  //...
} else {
  // Otherwise, create a temporary to hold the value.
  DeclPtr = CreateMemTemp(Ty, getContext().getDeclAlign(&D),
                          D.getName() + ".addr");
  DoStore = true;
}

We have this store before and after my change, and it doesn't depend on comparing ArgI.getCoerceToType() to other types.

The extra store+load comes from CreateCoercedStore and EmitLoadOfScalar a few lines lower. I've tried to avoid this store+load for cases where we can use only emitArgumentDemotion and CreateBitCast. But it doesn't look to be particularly simple and starts to look like incomplete version of "trivial case, handle it with no muss and fuss".

Knowing the cause of the extra store+load I don't think it is crucial to avoid it. I'll update the patch with ArgI.getCoerceToType() == ConvertType(Arg->getType()) comparison and fixed tests.

2461

Here is where we add the extra store.

[vsapsai]

• Address review comment, compare ArgI type with Arg type. Update tests to account for coerced store when types don't match.

[rjmccall]

Okay, I think that's reasonable enough. LGTM.

[vsapsai]

The fix wasn't entirely correct and was reverted in r321306 as it caused test failures

FAIL: imp.execution_time
FAIL: 2007-01-04-KNR-Args.execution_time
FAIL: sse_expandfft.execution_time
FAIL: sse_stepfft.execution_time

in LLVM test suite. Will push new revision with better fix shortly.

[vsapsai]

• Fix using CreateCoercedStore for promoted float parameters.

For promoted parameters double -> float conversion should use fptrunc, not
casting through memory preserving bits. Tried to change CreateCoercedStore to
do CreateFPCast but it caused CodeGen/arm-fp16-arguments.c to fail as it
expects __fp16 values to be passed in the least-significant 16 bits.

[vsapsai]

Add Akira as __fp16 expert to make sure I don't break some edge cases.

[ahatanak]

It doesn't look like this patch would break IRGen for functions that receive fp16 types. fp16 is passed as i32 or float (without promotion) on ARM and as half on ARM64. It should work fine.

[ahatanak]

clang/lib/CodeGen/CGCall.cpp

2322

Maybe a comment explaining why different types are passed depending on the value of isPromoted?

[vsapsai]

clang/lib/CodeGen/CGCall.cpp

2322

My reasoning was that for promoted arguments emitArgumentDemotion is responsible for bridging gaps between Ty and Arg->getType(), so it should be enough for ArgI.getCoerceToType() and Ty to match. But this explanation is woefully unsuitable for the comment.

After some more thinking I start doubting if some other s/Ty/Arg->getType()/ changes are entirely correct.

2469

For example, here. If V is of type Arg->getType(), will emitArgumentDemotion do anything? Because there you have

if (value->getType() == varType) return value;

I'll keep digging and if meanwhile anybody has comments on this situation, I'll be glad to hear them.

[vsapsai]

• Try another approach for the fix. It has broader impact but semantically seems to be more correct.

Also I think hasScalarEvaluationKind should use Arg->getType() everywhere
as it matches EmitParmDecl behaviour. Instead of updating all
hasScalarEvaluationKind calls I've added an assertion to verify it doesn't
matter if we call it with Arg->getType() or Ty.

[rjmccall]

LGTM.

clang/lib/CodeGen/CGCall.cpp

case ABIArgInfo::Indirect: {
       break;
     }
 
     case ABIArgInfo::Extend:
     case ABIArgInfo::Direct: {
 
       // If we have the trivial case, handle it with no muss and fuss.
       if (!isa<llvm::StructType>(ArgI.getCoerceToType()) &&
+          !isa<llvm::StructType>(ConvertType(Arg->getType())) &&
           ArgI.getCoerceToType() == ConvertType(Ty) &&
           ArgI.getDirectOffset() == 0) {
         assert(NumIRArgs == 1);
         llvm::Value *V = FnArgs[FirstIRArg];
         auto AI = cast<llvm::Argument>(V);
 
         if (const ParmVarDecl *PVD = dyn_cast<ParmVarDecl>(Arg)) {
           if (getNonNullAttr(CurCodeDecl, PVD, PVD->getType(),
                              PVD->getFunctionScopeIndex()))
             AI->addAttr(llvm::Attribute::NonNull);
case ABIArgInfo::Direct: {
         llvm::Type *LTy = ConvertType(Arg->getType());
         if (V->getType() != LTy)
           V = Builder.CreateBitCast(V, LTy);
 
         ArgVals.push_back(ParamValue::forDirect(V));
         break;
       }
 
-      Address Alloca = CreateMemTemp(Ty, getContext().getDeclAlign(Arg),
-                                     Arg->getName());
+      Address Alloca = CreateMemTemp(
+          Arg->getType(), getContext().getDeclAlign(Arg), Arg->getName());
 
       // Pointer to store into.
       Address Ptr = emitAddressAtOffset(*this, Alloca, ArgI);
 
       // Fast-isel and the optimizer generally like scalar values better than
       // FCAs, so we flatten them if this is safe to do for this argument.
       llvm::StructType *STy = dyn_cast<llvm::StructType>(ArgI.getCoerceToType());
       if (ArgI.isDirect() && ArgI.getCanBeFlattened() && STy &&
case ABIArgInfo::Direct: {
           Builder.CreateMemCpy(Ptr, AddrToStoreInto, DstSize);
         }
 
       } else {
         // Simple case, just do a coerced store of the argument into the alloca.
         assert(NumIRArgs == 1);
         auto AI = FnArgs[FirstIRArg];
         AI->setName(Arg->getName() + ".coerce");
         CreateCoercedStore(AI, Ptr, /*DestIsVolatile=*/false, *this);
       }
 
       // Match to what EmitParmDecl is expecting for this type.
-      if (CodeGenFunction::hasScalarEvaluationKind(Ty)) {
+      if (CodeGenFunction::hasScalarEvaluationKind(Arg->getType())) {
         llvm::Value *V =
-          EmitLoadOfScalar(Alloca, false, Ty, Arg->getLocStart());
+            EmitLoadOfScalar(Alloca, false, Arg->getType(), Arg->getLocStart());
         if (isPromoted)
           V = emitArgumentDemotion(*this, Arg, V);
         ArgVals.push_back(ParamValue::forDirect(V));
       } else {
         ArgVals.push_back(ParamValue::forIndirect(Alloca));
       }
       break;
     }
 
     case ABIArgInfo::CoerceAndExpand: {

clang/test/CodeGen/transparent-union-redecl.c

+// RUN: %clang_cc1 -Werror -triple i386-linux -emit-llvm -o - %s | FileCheck %s
+
+// Test that different order of declarations is acceptable and that
+// implementing different redeclarations is acceptable.
+// rdar://problem/34949329
+
+typedef union {
+  int i;
+  float f;
+} TU __attribute__((transparent_union));
+
+// CHECK-LABEL: define void @f0(i32 %tu.coerce)
+// CHECK: %tu = alloca %union.TU, align 4
+// CHECK: %coerce.dive = getelementptr inbounds %union.TU, %union.TU* %tu, i32 0, i32 0
+// CHECK: store i32 %tu.coerce, i32* %coerce.dive, align 4
+void f0(TU tu) {}
+void f0(int i);
+
+// CHECK-LABEL: define void @f1(i32 %tu.coerce)
+// CHECK: %tu = alloca %union.TU, align 4
+// CHECK: %coerce.dive = getelementptr inbounds %union.TU, %union.TU* %tu, i32 0, i32 0
+// CHECK: store i32 %tu.coerce, i32* %coerce.dive, align 4
+void f1(int i);
+void f1(TU tu) {}
+
+// CHECK-LABEL: define void @f2(i32 %i)
+// CHECK: %i.addr = alloca i32, align 4
+// CHECK: store i32 %i, i32* %i.addr, align 4
+void f2(TU tu);
+void f2(int i) {}
+
+// CHECK-LABEL: define void @f3(i32 %i)
+// CHECK: %i.addr = alloca i32, align 4
+// CHECK: store i32 %i, i32* %i.addr, align 4
+void f3(int i) {}
+void f3(TU tu);

clang/test/Sema/transparent-union.c

 void fsp(short *s) {} // expected-error{{conflicting types}}
 
 void fi(TU); // expected-note{{previous declaration is here}}
 void fi(int i) {} // expected-error{{conflicting types}}
 
 void fvpp(TU); // expected-note{{previous declaration is here}}
 void fvpp(void **v) {} // expected-error{{conflicting types}}
 
+/* Test redeclaring a function taking a transparent_union arg more than twice.
+   Merging different declarations depends on their order, vary order too. */
+
+void f_triple0(TU tu) {}
+void f_triple0(int *); // expected-note{{previous declaration is here}}
+void f_triple0(float *f); // expected-error{{conflicting types}}
+
+void f_triple1(int *);
+void f_triple1(TU tu) {} // expected-note{{previous definition is here}}
+void f_triple1(float *f); // expected-error{{conflicting types}}
+
+void f_triple2(int *); // expected-note{{previous declaration is here}}
+void f_triple2(float *f); // expected-error{{conflicting types}}
+void f_triple2(TU tu) {}
+
+/* Test calling redeclared function taking a transparent_union arg. */
+
+void f_callee(TU);
+void f_callee(int *i) {} // expected-note{{passing argument to parameter 'i' here}}
+
+void caller(void) {
+  TU tu;
+  f_callee(tu); // expected-error{{passing 'TU' to parameter of incompatible type 'int *'}}
+
+  int *i;
+  f_callee(i);
+}
+
+
 /* FIXME: we'd like to just use an "int" here and align it differently
    from the normal "int", but if we do so we lose the alignment
    information from the typedef within the compiler. */
 typedef struct { int x, y; } __attribute__((aligned(8))) aligned_struct8;
 
 typedef struct { int x, y; } __attribute__((aligned(4))) aligned_struct4;
 typedef union {
   aligned_struct4 s4; // expected-note{{alignment of first field}}