[llvm] r256620 - [ptr-traits] Implement the base pointer traits using the actual

Thu Dec 31 03:31:39 PST 2015

On 12/30/2015 10:13 PM, Chandler Carruth wrote:
> Ah, thanks, you got to it before I did. I'll test everything, check your
> patches, and re-land!

> I also (now that I'm using a real keyboard instead of a phone) wanted to
> more fully respond to your concerns about incomplete types. I think this
> is an important point, so its worth being thorough in the examination of
> the options here.

Yes, I think this is worth discussing.

> The reason to use an incomplete type, much as you indicate, is as some
> form of abstraction boundary: specifically, not relying on *any aspect*
> of how that type is implemented, merely that it is a type. Currently,
> the only thing you can rely on is that it isn't an enum or a primitive
> type. I actually think it would be great if the language even hid *that*
> detail from a forward declaration. They should (IMO) work very much like
> "typename T". You know its a type, but that is all.

Right.

> The question becomes, is it reasonable to use such a construct in a
> PointerIntPair (and consequentially in a PointerUnion or a DenseMap
> pointer key).
>
> I would argue that the only thing reasonable to do with such a type in a
> PointerIntPair is to declare the PointerIntPair. The data layout and
> size of such a PointerIntPair is unambiguous and clear. Similarly, it is
> reasonable to declare a pointer.
>
> But I think it is not reasonable to *use* the PointerIntPair any more
> than it is reasonable to dereference the pointer. Unfortunately, use of
> the PointerIntPair happens even when you assign, copy, or move it. But
> this is inherent in the nature of the beast: in order to know how to
> behave, PointerIntPair must know how the pointee type is *implemented*.
> So you can have a member, but you will need to define your methods that
> operate on it out-of-line and provide the full definition. This still
> allows circular references and other common patterns, but it is more
> restrictive than a raw pointer. I think that is, again, in the nature of
> the beast. We are fundamentally relying on more than just the fact that
> it is a pointer -- we rely on the particular alignment that pointer will
> always have.
>
> The example that fully convinced me of all of this goes as follows. When
> you forward declare a class, all uses of the forward declared class
> should be reasonable for *any and all possible implementations* of that
> class. There should never be a dependence on how the class implemented.
> But with PointerIntPair, if I replace the implementation of a class
> with, for example, a single unsigned char that is an index into a single
> table of all 255 possible instances, essentially forming a *very*
> compressed pimpl, it would actually break anyone using that class in a
> PointerIntPair. And it would do so silently in many cases! For me, this
> makes it really clear that PointerIntPair is actually depending on a
> particular implementation, and can't correctly work with an incomplete type.

I have not been aware until yesterday that DenseMap uses something like 
PointerIntPair and - looking at the code - it does not seem to actually 
use the PointerIntPair class. However, it seems to exploit unused bits 
to identify thumbstone keys, which is probably what you were referring
to in your explanation above.

The description of a DenseMap in the programmers manual [1] does not 
mention any requirement for aligned pointers and the benefits of a 
DenseMap do not seem to inherently require the availability of such 
unused bits, as a DenseMap<char *, ?> can be instantiated and will use 
NowLowBitsAvailable=0.

[1] http://llvm.org/docs/ProgrammersManual.html#llvm-adt-densemap-h

As without this information, the need for complete types is slightly 
surprising (and different to std::map, std::vector), it might be helpful
to add a brief explanation to the LLVM Programmers manual.

For use cases where the type definition (and consequently the number of 
available bits) is not available, it might make sense to provide a
AlignedDenseMapInfo<T*, NowLowBitsAvailable> that can be provided as 
KeyInfoT in case the user wants (needs) to explicitly specify an 
alignment. This would allow me to specify for incomplete types an
explicit alignment without risking ODR violations:

struct IncompleteType;

llvm::DenseMap<IncompleteType *, llvm::Value,
     llvm::AlignedDenseMapInfo<IncompleteType *, NowLowBitsAvailable>>>

For LLVM, we may not need this in case we agree (and document?) that we 
always want full type definitions to be available. If this is the case, 
I could also introduce such an AlignedDenseMapInfo in Polly.

To illustrate the use of AlignedDenseMapInfo, I attached patches for 
LLVM/clang/Polly.

> Hope this helps explain my reasoning, and thanks again for the very
> quick polly fixes, and apologies again for not getting polly fixed first.
> -Chandler

Sorry also for reverting the patch. I was not on a computer for long 
enough at that time such that my primary concern was to avoid red 
buildbots that send out emails until I have the time to actually reason 
about this issue.

Best,
Tobias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: LLVM-Introduce-AlignedDenseMapInfo.patch
Type: text/x-patch
Size: 4934 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151231/a8a8451e/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-Ensure-types-needed-for-DenseMap-are-fully-defined.patch
Type: text/x-patch
Size: 1764 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151231/a8a8451e/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Polly-Use-AlignedDenseMapInfo.patch
Type: text/x-patch
Size: 1484 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151231/a8a8451e/attachment-0002.bin>