<br><br><div class="gmail_quote">On Mon, Aug 8, 2011 at 2:58 AM, Renato Golin <span dir="ltr"><<a href="mailto:renato.golin@arm.com">renato.golin@arm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Ping?<br>
<div><div></div><div class="h5"><br></div></div></blockquote><div><br></div><div><br></div><div>Hi Renato</div><div><br></div><div>Patch looks okay --- </div><div>Just a couple of nits. Please fix and commit.</div><div><br>
</div><div>1. no need to typedef a anon struct AttributeItem - this is C++ (enter appropriate "300" joke here)<br>2. Are you sure there is not a better way to null terminate the string then + Streamer.EmitIntValue(0, 1); // '\0'</div>
<div>3. also, in getULEBSize() you are doing a signed right shift. Is that correct?</div><div>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)</div>
<div><br></div><div>It wastes memory, but given that we probably will not see lots (as in millions) of attributes, its probably ok...</div><div><br></div><div>Thanks</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div class="h5">
On 1 August 2011 13:20, Renato Golin <<a href="mailto:renato.golin@arm.com">renato.golin@arm.com</a>> wrote:<br>
> Hi all,<br>
><br>
> I've been playing with a FIXME in the ARM back-end (to get more<br>
> acquainted) and one simple FIXME was an ULEB printing in the<br>
> AsmPrinter.<br>
><br>
> According to the ARM EABI, both build attribute tag and value are<br>
> ULEBs and not chars as they were being emitted. Today, I don't know of<br>
> any attribute that would emit tags or values bigger than 127 but this<br>
> seamed like an easy start.<br>
><br>
> The patch changes the emission of values to ULEB by storing all tags<br>
> and values in a typed list and emit later the right value, computing<br>
> the size as it goes. I've run all tests (including the build<br>
> attributes ones in ARM be) and it passes everything.<br>
><br>
> There are three problems I see with this patch, but would like to know<br>
> from you what's the best way to solve them:<br>
><br>
> 1. It uses a local structure to hold both ULEB and String values in<br>
> order, but other MC structures will also emit ULEB in the near future<br>
> (we should make sure it does, at least), so this structure could<br>
> become a proper small class on a more visible place.<br>
><br>
> 2. It wastes memory since both number and string values are on every<br>
> item, but it's not possible to put StringRef in an union since<br>
> Stringref has non-trivial constructors. A solution is to use<br>
> polymorphism but that's too big for this tiny case. If we decide to<br>
> expose the class higher up, then it makes sense to do so.<br>
><br>
> 3. It's calculating the ULEB size in place, where other routines in<br>
> LLVM can do that (but are not accessible from this class). A<br>
> refactoring is in need to expose that routine to the intended<br>
> audience.<br>
><br>
> If neither of those problems are relevant, or if they're just minor,<br>
> we can put other FIXMEs around. Otherwise, I'm open to suggestions.<br>
><br>
> best,<br>
> --renato<br>
><br>
<br>
<br>
<br>
</div></div><font color="#888888">--<br>
cheers,<br>
--renato<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</font></blockquote></div><br>