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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2321
+          !isa<llvm::StructType>(ConvertType(Arg->getType())) &&
           ArgI.getCoerceToType() == ConvertType(Ty) &&
           ArgI.getDirectOffset() == 0) {
----------------
rjmccall wrote:
> 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.
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.


https://reviews.llvm.org/D41311