[patch] mergefunc struct result

[Duncan P. N. Exon Smith]

Hi Carlo,

Thanks for working on this.  See comments below.

On 2014-Apr-18, at 11:21, Carlo Kok <ck at remobjects.com> wrote:

> When using the merge func pass with two functions that return the same
> kind of struct but with different element types merge func tries to do a
> bitcast of struct to struct and fails with an assert (as you can't bitcast a struct) given the following functions:
> 
> %kv1 = type { i32*, i32* }
> %kv2 = type { i8*, i8* }
> 
> define %kv1 @fn1() ..
> define %kv2 @fn2() ..
> 
> (full file in fail.ir)

This testcase should be part of the commit.  The file extension for LLVM IR is
".ll".  You should find a good place to put this (probably
test/Transforms/MergeFunc/) and give it a descriptive name (maybe
merge-structs.ll?).

There's a good overview of the testing infrastructure here:

http://llvm.org/docs/TestingGuide.html

> 
> The attached patch checks for two struct types & uses insert/extract to
> create the right record.
> 
> To see it fail, fail.ir can be used with opt:
> 
> opt -mergefunc fail.ir -o out.ir

You'll want to add a RUN line something like:

; RUN: opt -mergefunc < %s | FileCheck %s

> after the patch it generates:
> 
> define %kv1 @fn1() {
>  %tmp = alloca %kv1
>  %v1 = getelementptr %kv1* %tmp, i32 0, i32 0
>  store i32* null, i32** %v1
>  %v2 = getelementptr %kv1* %tmp, i32 0, i32 0
>  store i32* null, i32** %v2
>  call void @noop()
>  %v3 = load %kv1* %tmp
>  ret %kv1 %v3
> }
> 
> define %kv2 @fn2() {
>  %1 = tail call %kv1 @fn1()
>  %2 = extractvalue %kv1 %1, 0
>  %3 = bitcast i32* %2 to i8*
>  %4 = insertvalue %kv2 undef, i8* %3, 0
>  %5 = extractvalue %kv1 %1, 1
>  %6 = bitcast i32* %5 to i8*
>  %7 = insertvalue %kv2 %4, i8* %6, 1
>  ret %kv2 %7
> }
> 
> Where fn2 is the merged function.
> 

And then some CHECK lines to confirm that the output is correct.

On the patch itself:

 lib/Transforms/IPO/MergeFunctions.cpp | 10 ++++++++++
 1 file changed, 10 insertions(+)

> diff --git a/lib/Transforms/IPO/MergeFunctions.cpp b/lib/Transforms/IPO/MergeFunctions.cpp
> index 8555d2c..b4a9121 100644
> --- a/lib/Transforms/IPO/MergeFunctions.cpp
> +++ b/lib/Transforms/IPO/MergeFunctions.cpp

Style doesn't match our agreed upon standards [1].  clang-format [2] would fix
a lot of the issues.  Most of my comments below are style nitpicks, so I
encourage you to read the standards.

[1]: http://llvm.org/docs/CodingStandards.html
[2]: http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

> @@ -785,6 +785,16 @@ void MergeFunctions::writeThunkOrAlias(Function *F, Function *G) {
>  // but a bit simpler then CastInst::getCastOpcode.
>  static Value* createCast(IRBuilder<false> &Builder, Value *V, Type *DestTy) {
>    Type *SrcTy = V->getType();
> +  if (SrcTy->isStructTy() && DestTy->isStructTy()) {

Should this be asserting:

    assert(DestTy->isStructTy())

rather than checking it?

> +    assert(SrcTy->getStructNumElements() == DestTy->getStructNumElements());
> +    Value* res = UndefValue::get(DestTy);

Should be capitalized and the * is placed wrongly.

    Value *Result = UndefValue::get(DestTy);

> +    for (unsigned int i = 0; i < SrcTy->getStructNumElements(); i++) {

Should be:

    for (unsigned int I = 0, E = SrcTy->getStructNumElements(); I < E; ++I)

Capitilized variables, evaluate the end condition once, pre-increment, and no
need for {} on a single statement.

> +      res = Builder.CreateInsertValue(res, createCast(Builder, 
> +        Builder.CreateExtractValue(V, { i }), 
> +        DestTy->getStructElementType(i)), { i });

I'm not sure MSVC will accept this initializer list.  Regardless, ArrayRef has
a constructor from element type, so you can just leave off the braces.

Also, this line is a little complicated.  Maybe you should split it into
two statements (separate the createCast() from the CreateInsertValue()).

If you leave it as is, the continuation should be double-indented (i.e.,
four spaces), and should probably look more like this:

    Result = Builder.CreateInsertValue(
        Result, createCast(Builder, Builder.CreateExtractValue(V, I), 
                           DestTy->getStructElementType(i)),
        I);

> +    return res;
> +  }
>    if (SrcTy->isIntegerTy() && DestTy->isPointerTy())
>      return Builder.CreateIntToPtr(V, DestTy);
>    else if (SrcTy->isPointerTy() && DestTy->isIntegerTy())