[llvm-commits] [PATCH] PR12696: Encode 'nonlazybind' and 'address_safety' properly in BitcodeWriter

[Chris Lattner]

On May 4, 2012, at 6:13 PM, Meador Inge wrote:
>>> This patch fixes PR12696 which reported that the BitcodeWriter was not
>>> encoding the
>>> 'nonlazybind' and 'address_safety' attributes correctly.  Alessandro
>>> Pignotti suggested
>>> the original patch.  I wrote the test case and regression tested the change.
>> 
>> This looks very close, but I don't like the magic numbers in the bitcode writer.  If there is no convenient enum in Attributes.h, can you add a new enum
>> with the magic number there?
> 
> Which magic numbers, though?

Sorry, I meant this sort of thing, which you've factored out in the latest patch:

-        unsigned Alignment = (Record[i+1] & (0xffffull << 16)) >> 16;
-        if (Alignment && !isPowerOf2_32(Alignment))
-          return Error("Alignment is not a power of two.");

Those magic numbers were based on the way that we currently encode Alignment, with:

DECLARE_LLVM_ATTRIBUTE(Alignment,31<<16) ///< Alignment of parameter (5 bits)
                                     // stored as log2 of alignment with +1 bias
                                     // 0 means unaligned different from align 1


> In this particular case there are many
> magic numbers used for shifting and masking.  The complication arises
> from the fact that we are picking apart the attribute set to expand
> the alignment and then encoding that expanded alignment with the other
> attributes (a similar problem for decoding exist in the bitcode
> reader).  If the magic numbers were to be replaced for this case, then
> you would probably want enum/constants for all the masks and shifts.
> I think that is overkill.  Also, you can only go so for hiding the
> magic for bit twiddling stuff like this.

Yes, agreed.

> One alternative is to hoist the operations out of the bitcode
> implementation into encoding/decoding inline functions in
> Attributes.h.  This way the bit twiddling is consolidated into a
> single place as a part of the attributes implementation (which we
> already do for similar operations like constructing alignments).  The
> attached patch implements this approach and extends the test case to
> cover all function and parameter attributes.

Looks good to me, one minor change:

+/// This returns an integer containing an encoding of all the
+/// LLVM attributes found in the given attribute bitset.
+inline uint64_t encodeLLVMAttributes(Attributes Attrs) {

Please rename this to "encodeLLVMAttributesForBitcode" (likewise "decode") and add a comment that this cannot change without breaking bitcode compatibility.  These functions are a bc specific encoding that we don't to be used for other things. 


> Also, maybe addressing the "// FIXME: remove in LLVM 3.0" FIXMEs would
> simplify things.  Although, I am not completely sure what needs to be
> done to address those FIXMEs.  If (when) they are addressed at least
> with the inline function approach the places being fixed will be right
> next to each other :-)

Given that we didn't fix it in LLVM 3.0, we lost our chance.  :)  Please change the fixme (in both functions) to something like:

// FIXME: It doesn't make sense to store the alignment information as an expanded out value, we should store it as a log2 value.  However, we can't just change that here without breaking bitcode compatibility.  If this ever becomes a problem in practice, we should introduce new tag numbers in the bitcode file and have those tags use a more efficiently encoded alignment field.

Please commit the patch with these changes, thanks Meador!

-Chris