[cfe-commits] [PATCH/RFC] Extend TBAA to handle OpenCL address spaces

[Peter Collingbourne]

On Fri, Oct 14, 2011 at 05:11:31PM -0700, Justin Holewinski wrote:
> Attached is an updated patch.  Any more comments before I commit?

I have just realised that OpenCL C should use its own TBAA root,
because if we link OpenCL C and C code into the same module, the
OpenCL C types may-alias the C types.  The C TBAA root should also
be used for address spaces outside of the range of defined OpenCL
address spaces (the implementation may use them), so that all unknown
address spaces may-alias.

> +  for (unsigned i = 0; i < LangAS::Count+1; ++i) {
> +    CharCache[i] = 0;
> +  }

For the reasons above, this should actually be LangAS::Count+2.

> +    // If we are in OpenCL mode, add an address space identifier to the name.
> +    if (Context.getLangOptions().OpenCL) {
> +      switch (Quals.getAddressSpace()) {
> +      default:
> +        llvm_unreachable("Unknown OpenCL address space");

This is not unreachable if the implementation uses an unknown address
space.  If we see this, we should use C TBAA's "onmipotent char".

> +      case 0:
> +        Name += " - OCL default";
> +        break;
> +      case LangAS::opencl_global:
> +        Name += " - OCL global";
> +        break;
> +      case LangAS::opencl_local:
> +        Name += " - OCL local";
> +        break;
> +      case LangAS::opencl_constant:
> +        Name += " - OCL constant";
> +        break;
> +      }
> +    }
> +    CharCache[CacheIndex] = getTBAAInfoForNamedType(Name, getRoot());
> +  }
> +  return CharCache[CacheIndex];
> +}
> +
> +unsigned CodeGenTBAA::getCacheIndex(Qualifiers Quals) {
> +  unsigned AS = 0;
> +  if (Context.getLangOptions().OpenCL) {
> +    AS = Quals.getAddressSpace();
> +    if (AS < LangAS::Offset || AS >= LangAS::Offset + LangAS::Count)
> +     return AS;

You should return 1 here, and add a case for address space 0 (in
which case you should return 0).  Otherwise the function may return
a value greater than LangAS::Count, and address spaces 1-4 would
incorrectly may-alias the OpenCL address spaces (and may-not-alias
address space 0).

> +    else
> +      return AS - LangAS::Offset;

This should be AS - LangAS::Offset + 2.

> +  }
> +  return AS;
>  }
>  
>  /// getTBAAInfoForNamedType - Create a TBAA tree node with the given string
> @@ -94,16 +133,24 @@
>    return false;
>  }
>  
> -llvm::MDNode *
> -CodeGenTBAA::getTBAAInfo(QualType QTy) {
> +llvm::MDNode *CodeGenTBAA::getTBAAInfo(QualType QTy) {
>    // If the type has the may_alias attribute (even on a typedef), it is
>    // effectively in the general char alias class.
>    if (TypeHasMayAlias(QTy))
> -    return getChar();
> +    return getChar(QTy.getQualifiers());
> +  else
> +    return getTBAAInfo(QTy.split());

This should be QTy.getCanonicalType().split(), to avoid recomputing
the canonical type in the function below.

> +}
>  
> -  const Type *Ty = Context.getCanonicalType(QTy).getTypePtr();
> +llvm::MDNode *CodeGenTBAA::getTBAAInfo(SplitQualType QTy) {
> +  // Retrieve the canonical type and the qualifiers for this QualType.
> +  const Type *Ty = Context.getCanonicalType(QTy.first);

You should just use QTy.first here.

Other than that, go ahead.

Thanks,
-- 
Peter