[llvm-commits] [PATCH] ULEB FIXME in ARM back-end

[Jason Kim]

On Mon, Aug 8, 2011 at 2:58 AM, Renato Golin <renato.golin at arm.com> wrote:

> Ping?
>
>

Hi Renato

Patch looks okay ---
Just a couple of nits. Please fix and commit.

1. no need to typedef a anon struct AttributeItem - this is C++ (enter
appropriate "300" joke here)
2. Are you sure there is not a better way to null terminate the string
then +          Streamer.EmitIntValue(0, 1); // '\0'
3. also, in getULEBSize() you are doing a signed right shift. Is that
correct?
4. also I like size_t when you have to return size of something - (in case
some crazy person wants to emit an attribute more than 2^32 bytes long)

It wastes memory, but given that we probably will not see lots (as in
millions) of attributes, its probably ok...

Thanks




>  On 1 August 2011 13:20, Renato Golin <renato.golin at arm.com> wrote:
> > Hi all,
> >
> > I've been playing with a FIXME in the ARM back-end (to get more
> > acquainted) and one simple FIXME was an ULEB printing in the
> > AsmPrinter.
> >
> > According to the ARM EABI, both build attribute tag and value are
> > ULEBs and not chars as they were being emitted. Today, I don't know of
> > any attribute that would emit tags or values bigger than 127 but this
> > seamed like an easy start.
> >
> > The patch changes the emission of values to ULEB by storing all tags
> > and values in a typed list and emit later the right value, computing
> > the size as it goes. I've run all tests (including the build
> > attributes ones in ARM be) and it passes everything.
> >
> > There are three problems I see with this patch, but would like to know
> > from you what's the best way to solve them:
> >
> >  1. It uses a local structure to hold both ULEB and String values in
> > order, but other MC structures will also emit ULEB in the near future
> > (we should make sure it does, at least), so this structure could
> > become a proper small class on a more visible place.
> >
> >  2. It wastes memory since both number and string values are on every
> > item, but it's not possible to put StringRef in an union since
> > Stringref has non-trivial constructors. A solution is to use
> > polymorphism but that's too big for this tiny case. If we decide to
> > expose the class higher up, then it makes sense to do so.
> >
> >  3. It's calculating the ULEB size in place, where other routines in
> > LLVM can do that (but are not accessible from this class). A
> > refactoring is in need to expose that routine to the intended
> > audience.
> >
> > If neither of those problems are relevant, or if they're just minor,
> > we can put other FIXMEs around. Otherwise, I'm open to suggestions.
> >
> > best,
> > --renato
> >
>
>
>
> --
> cheers,
> --renato
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110808/2c9c1c87/attachment.html>