[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) {
----------------
vsapsai wrote:
> 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.
The last store comes from `CodeGenFunction::EmitParmDecl` where for direct arguments we have
```lang=c++
  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.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2461
         AI->setName(Arg->getName() + ".coerce");
         CreateCoercedStore(AI, Ptr, /*DestIsVolatile=*/false, *this);
       }
----------------
Here is where we add the extra store.


https://reviews.llvm.org/D41311