[llvm-commits] [PATCH] MCFragments Clean Up

[David Meyer]

Hi Dan,

I'm not sure what you mean by vtable construction cost. Do you mean the
additional memory to store the vptr? I can't deny that a switch-based
implementation uses a little less memory and is probably slightly faster
(one less memory read & a closer jump on dispatch). However, I think the
resulting code could be significantly cleaner and more extensible.

As you can probably tell, I've been working on adding native-client
instruction bundling to the MC assembler. The modifications are almost
finished. Unfortunately, it has been quite an unpleasant experience.

One of the things I did was to add a new fragment type which can store
multiple instructions (usually about 3), each with its own fixups and
(possibly) each needing relaxation separately. The existing relaxation and
lowering mechanism assumes that only MCInstFragment needs relaxation and
lowering. I had to copy & paste & modify most of the relaxation code in
order to make it relax my new fragment type. If there were instead virtual
methods NeedRelaxation() and PerformRelaxation() and LowerFragment() on
fragments, this would not have been necessary.

I also needed to control the creation of fragments. With bundling enabled,
the creation of a fragment sometimes needs to trigger the substitution for a
different type of fragment. Unfortunately, the creation of fragments (e.g.
"new MCAlignFragment(...)") is sprinkled all over the MCObjectStreamer
subclasses instead of being private to MCAssembler. This made the required
changes messy and spread out.

I'm curious as to why the the assembler doesn't keep fragments as private
data. Instead, they get passed and manipulated all over the place. Fragments
even get passed directly to the TargetAsmBackend! The number of places in
the code that "knows" about a fragments is alarming.

I think that ideally, fragment creation could have been hidden behind
functions like:

// Emits into MCInstFragment or into the current MCDataFragment
MCAssembler::EmitInstruction(MCSectionData *Parent, const MCInst &Inst);

MCAssembler::EmitValueToAlignment(...);

MCAssember::EmitCodeAlignment(...);

MCAssembler::EmitOrg(...);

MCAssembler::EmitDwarf(...);

I'd like to talk more about this. I'd like to get the bundling feature clean
enough to be added to mainstream LLVM MC. The changes are mostly innocuous,
but little things are getting in the way of making the implementation
compact. Unfortunately, I'm under intense pressure to get this stuff working
a week ago (in our local branch). I can't focus on refactoring at the
moment.

When I'm finished, I'll send you a ping.

Thanks,
  David M


On Fri, Nov 5, 2010 at 11:09 AM, Daniel Dunbar <daniel at zuster.org> wrote:

> Hi David,
>
> MCFragment actually used to be virtual.
>
> I eventually killed this because in practice I felt like the number of
> interesting fragments was likely to be small, and the vtable
> construction cost wasn't worth it.
>
> I would propose making an extensible MCFragment subclass to cover the
> case of wanting to add plugable MCFragments, instead of imposing a
> vtable construction cost on the core classes (which was construct many
> hundreds of, and don't actually dispatch on much).
>
>  - Daniel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101110/4e2059c5/attachment.html>