Use unique'ed types for varargs expansion instead of non-unique named types.

Use unique'ed types for varargs expansion instead of non-unique named types.

Saves a tiny bit of space for var-args heavy bitcode
programs, since the anonymous types get unique'ed.  E.g.,
saves 20KB out of 1.6MB in spec gcc when comparing the
gzipped files, or about 100KB when not zipped.  This is only
a savings with the current wire format.  If we change the
alloca, etc. to only have sizes and not struct types
then we would also not have this duplication.

Just happened to notice while looking through code for
what struct types remain used in the bitcode.

random cleanup for:
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3338
R=mseaborn@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=1174111

[Mark Seaborn, 2013-04-11 23:15:29]

Doesn't this reduce readability of the partially-expanded IR?

Since the plan is to expand out struct types, this wouldn't affect the size of
the wire-format pexes.

For testability of the expanded IR, this seems neutral.  The asm output is more
localised (not divided between code + type decl) but more verbose.

If we were to keep struct types in the pexe, surely we'd want to convert *all*
named struct types to non-uniqued, interned struct types?

https://codereview.chromium.org/14197004/diff/1/test/Transforms/NaCl/expand-v...
File test/Transforms/NaCl/expand-varargs.ll (right):

https://codereview.chromium.org/14197004/diff/1/test/Transforms/NaCl/expand-v...
test/Transforms/NaCl/expand-varargs.ll:52: ; We have a dummy field to deal with
buggy programs:
Maybe put this just before "%vararg_buffer = ..." and write "We have a dummy i32
field...".

[jvoung (off chromium), 2013-04-12 00:37:40]

On 2013/04/11 23:15:29, Mark Seaborn wrote:
> Doesn't this reduce readability of the partially-expanded IR?
> 


Well, for unstripped bitcode, each of these named types are tied to the
%vararg_buffer variables, so it will be clear that they are related to the
vararg expansion pass.  It can be more verbose though if the buffer has many
elements.


For stripped bitcode, the types are "named" for the sake of semantics (they
cannot be uniqued) but they are just given numbers.


> Since the plan is to expand out struct types, this wouldn't affect the size of
> the wire-format pexes.

That's true.  This probably won't matter when the format changes.  In that case,
the alloca would just have the size and not the struct type?  

Anyway, given that, I'll probably withdraw this CL.


> 
> For testability of the expanded IR, this seems neutral.  The asm output is
more
> localised (not divided between code + type decl) but more verbose.
> 
> If we were to keep struct types in the pexe, surely we'd want to convert *all*
> named struct types to non-uniqued, interned struct types?
> 


What do you mean by non-uniqued, but interned?


>
https://codereview.chromium.org/14197004/diff/1/test/Transforms/NaCl/expand-v...
> File test/Transforms/NaCl/expand-varargs.ll (right):
> 
>
https://codereview.chromium.org/14197004/diff/1/test/Transforms/NaCl/expand-v...
> test/Transforms/NaCl/expand-varargs.ll:52: ; We have a dummy field to deal
with
> buggy programs:
> Maybe put this just before "%vararg_buffer = ..." and write "We have a dummy
i32
> field...".

[Mark Seaborn, 2013-04-12 19:27:23]

On 11 April 2013 17:37, <jvoung@chromium.org> wrote:

> On 2013/04/11 23:15:29, Mark Seaborn wrote:
>
>> Doesn't this reduce readability of the partially-expanded IR?
>>
>
>
> Well, for unstripped bitcode, each of these named types are tied to the
> %vararg_buffer variables, so it will be clear that they are related to the
> vararg expansion pass.  It can be more verbose though if the buffer has
> many
> elements.
>

That's true.


> For stripped bitcode, the types are "named" for the sake of semantics (they
> cannot be uniqued) but they are just given numbers.
>
>
>
>  Since the plan is to expand out struct types, this wouldn't affect the
>> size of
>> the wire-format pexes.
>>
>
> That's true.  This probably won't matter when the format changes.  In that
> case,
> the alloca would just have the size and not the struct type?
>

Yes, that's what the alloca would have.


>
>  For testability of the expanded IR, this seems neutral.  The asm output
>> is more
>>
> localised (not divided between code + type decl) but more verbose.
>>
>
>  If we were to keep struct types in the pexe, surely we'd want to convert
>> *all*
>> named struct types to non-uniqued, interned struct types?
>>
>
> What do you mean by non-uniqued, but interned?
>

Oh, I meant uniqued and interned, sorry.  Uniqued == interned.

(LLVM's use of the term "uniqued" throws me a little because I expect it to
mean that "struct Foo" and "struct Bar" are unique -- that is, distinct,
with their own identities.  But LLVM uses it to mean the opposite -- that
is, interned.  I'll have to get used to this.)

So, *if* we were to keep struct types in the pexe, we could convert
non-uniqued struct types into uniqued struct types.  This would weaken the
type checking, but save space.

LGTM if you want to commit this.  I just wanted to make sure we're on the
same page that, with the current plans, this wouldn't save space in the
wire format.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[jvoung (off chromium), 2013-04-29 20:56:20]

I think I'll commit this, just so that size comparisons from when we do switch
over will be more fair.

https://codereview.chromium.org/14197004/diff/1/test/Transforms/NaCl/expand-v...
File test/Transforms/NaCl/expand-varargs.ll (right):

https://codereview.chromium.org/14197004/diff/1/test/Transforms/NaCl/expand-v...
test/Transforms/NaCl/expand-varargs.ll:52: ; We have a dummy field to deal with
buggy programs:
On 2013/04/11 23:15:29, Mark Seaborn wrote:
> Maybe put this just before "%vararg_buffer = ..." and write "We have a dummy
i32
> field...".

Done.

[jvoung (off chromium), 2013-04-29 22:52:51]

Committed patchset #2 manually as r1174111 (presubmit successful).