[PATCH] Add support for symbolic large constant entries inside stackmaps
listmail at philipreames.com
Tue Jun 16 15:23:19 PDT 2015
In http://reviews.llvm.org/D9176#161738, @undingen wrote:
> I reused the existing stackmap format in order to not break existing clients. That's why I choose to put them in the constant value table but this means it's impossible for clients to retrieve the number of large constants which don't need symbol resolving and which do. (They will just be at the end of the table but there is no way to know how many there are at the end)
I would strongly prefer you extended the format. Reusing the existing sections for mixed purposes without updating the documentation would be a really bad idea.
> If changing the stackmap format is not a big problem than maybe it's best to make a new section, which would make the resulting stackmap and the code to generate it inside llvm more obvious.
The section format is versioned. Adding a new section is not a big deal.
> The MVT::i64 check is my attempt at making sure that the symbol addresses are exactly 64bit long like the entries in the stackmap constant table. But this may should get changed to allow smaller values.
This is an incompatible assumption with non-x86 bit architectures right? I'd prefer not to see this baked in.
> Thanks again for the comments, I will wait with changing the patch until we all agreed on how to proceed.
I would suggest that you update *just the docs* for the next couple of iterations. Until we stablize on what the feature will look like, getting the code exactly right will be a waste of time.
I say this specifically because I don't feel like I understand exactly what you're trying to propose. Until that part is clear, I can't really offer anything in the way of useful review.
More information about the llvm-commits