[llvm-commits] [PATCH] MCFragments Clean Up

[Rafael Espíndola]

On 10 November 2010 17:31, David Meyer <pdox at google.com> wrote:
> 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 changed that when I implemented uleb128 with expressions, but it is
not pretty. One of the things I was planning to do with your
refactoring was add a base class for all classes that are relaxed.

> 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.

Yes, I haven't put a lot of thought on how to improve this, but it
does look like that areas of the code that just need to append data
are handling fragments just because that is the alternative they have
for when the streamer is not available.

The ELFObjectWritter for example would probably be more than happy
with any API that lets it push data to the end of a given section.

> 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(...);

This looks a lot like the MCStreamer class. Maybe it should just be
available in more places?

> 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 a lot!

> Thanks,
>   David M

Cheers,
Rafael