[llvm-commits] PATCH: refactoring MCInstFragment and MCDataFragment

[Jim Grosbach]

This looks pretty reasonable to me.

A small question, though. The default size of the SmallString in MCInstFragment increased from 8 to 32. Why is that? That's where your object size change came from.

-Jim

On Dec 6, 2012, at 9:20 AM, Eli Bendersky <eliben at google.com> wrote:

> Hello,
> 
> This is a proof-of-concept refactoring patch for MCInstFragment and
> MCDataFragment. Since they have similar characteristics, I made them
> both implement an interface called MCEncodedFragment (suggestions for
> a better name will be most welcome). This removes the ugly code
> duplication in clients of these classes, which can now use this
> interface to do operations which make sense for both Data and Inst
> fragments.
> 
> The object sizes were affected as follows (x64 host):
> 
> empty MCDataFragment: 240 bytes before change, 240 after
> empty MCInstFragment: 320 bytes before change, 344 after
> 
> While the increase is insignificant, it can be solved by having the
> fragments hold their data in a SmallVector rather than SmallString.
> This way the interface can use SmallVectorImpl to escape being tied to
> size. Personally I think a SmallVector is more appropriate for such
> uses anyway. This can be implemented in a follow-up patch if
> requested.
> 
> Note that an alternative design of each MCDataFragment having an
> optional MCInst would result in significant size increase for the
> object and increased memory usage for assembling large object files.
> 
> If this looks reasonable, I will put comments in the code to explain
> the design and the interface.
> 
> P.S. As a bonus, such a design can align well with the plan to
> implement aligned instruction bundle support in MC
> (http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-December/056754.html),
> which will likely require an additional type of "encoded fragments".
> 
> Eli
> <refactor-mc-datainst-fragments.patch>