[polly] r256650 - IslExprBuilder: Provide PointerLikeTypeTraits for isl_id

[Tobias Grosser via llvm-commits]

On 12/30/2015 10:17 PM, Chandler Carruth wrote:
> On Wed, Dec 30, 2015 at 12:15 PM Tobias Grosser via llvm-commits
> <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>
>     Author: grosser
>     Date: Wed Dec 30 14:11:48 2015
>     New Revision: 256650
>
>     URL: http://llvm.org/viewvc/llvm-project?rev=256650&view=rev
>     Log:
>     IslExprBuilder: Provide PointerLikeTypeTraits for isl_id
>
>     Providing an explicit PointerLikeTypeTraits implementation became
>     necessary
>     since LLVM started in
>     https://llvm.org/svn/llvm-project/llvm/trunk@256620
>     to automatically derive the pointer alignment from the pointer
>     element type,
>     which does not work for incomplete types as used by isl. To ensure
>     our code
>     still compiles, we provide an instantiation of PointerLikeTypeTraits
>     for isl_id
>     which assumes no minimal alignment. isl pointers are likely to have
>     a "higher"
>     alignment. We can exploit this later in case this becomes
>     performance relevant.
>
>     Modified:
>          polly/trunk/include/polly/CodeGen/IslExprBuilder.h
>
>     Modified: polly/trunk/include/polly/CodeGen/IslExprBuilder.h
>     URL:
>     http://llvm.org/viewvc/llvm-project/polly/trunk/include/polly/CodeGen/IslExprBuilder.h?rev=256650&r1=256649&r2=256650&view=diff
>     ==============================================================================
>     --- polly/trunk/include/polly/CodeGen/IslExprBuilder.h (original)
>     +++ polly/trunk/include/polly/CodeGen/IslExprBuilder.h Wed Dec 30
>     14:11:48 2015
>     @@ -23,6 +23,21 @@ class DataLayout;
>       class ScalarEvolution;
>       }
>
>     +struct isl_id;
>     +
>     +namespace llvm {
>     +// Provide PointerLikeTypeTraits for isl_id.
>     +template <> class PointerLikeTypeTraits<isl_id *> {
>
>
> Unfortunately, this almost guarantees an ODR violation if anyone ever
> causes an instantiation of the primary template without having included
> this header.

Yes, I was afraid of such kind of issues here.

My reasoning was that in practice this might be pretty save (but rather 
ugly).

I found this piece of C++ on stackoverflow:

======================
§14.7.3 [temp.expl.spec]/p6 (emphasis added):

     If a template, a member template or a member of a class template is 
explicitly specialized then that specialization shall be declared before 
the first use of that specialization that would cause an implicit 
instantiation to take place, in every translation unit in which such a 
use occurs; no diagnostic is required. If the program does not provide a 
definition for an explicit specialization and either the specialization 
is used in a way that would cause an implicit instantiation to take 
place or the member is a virtual member function, the program is 
ill-formed, no diagnostic required.
======================

As we do not have a definition of isl_id available outside of isl 
itself, an instantiation of a PointerLikeTypeTraits without the 
specialization above results in a compile time error. So we would always
be warned if this instantiation would be needed, but never accidentally
instantiate a generic version of this template for isl_id.

> Why does isl_id need to remain incomplete here?

Because the type of isl_id is not exposed in any of the public headers 
of isl. I could probably include private headers, but this would require 
to make such headers available by adding additional include paths, ... 
As they are private on purpose, this seems to be just a bad hack.

> If it truly needs to be opaque, I would suggest changing whatever data
> structure to explicitly operate on void* which we provide a generic
> template for which assumes at least 4-byte alignment. This template is
> intended precisely for the cases where the types of the pointers change,
> aren't possibly known ahead of time, etc., and the code actually must
> erase the type and make generic assumptions.
>
> I find using void* to do this somewhat more transparent. What do you think?

This would mean we loose static type checking, clang auto-completion, 
and the code becomes also more difficult to read as we would need to 
document what void* actually means.

Finally, as stated in your other email, we can theoretically not even 
rely on the pointer to be 4 bit aligned. (This is why I set the number 
of free bits to 0).

I was thinking to move to standard C++ data structures, but 
unfortunately they do not provide a MapVector. This solution - despite 
not very beautiful - seems to work and be reasonably save. In the 
optimal case I would hope that DenseMap/MapVector/... could be used
without them exploiting pointer alignment. Let's keep this discussion in 
the other thread.

Best,
Tobias