Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 42 additions & 9 deletions clang/include/clang/AST/SYCLKernelInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,25 @@ namespace clang {

class SYCLKernelInfo {
public:
SYCLKernelInfo(
CanQualType KernelNameType,
const FunctionDecl *KernelEntryPointDecl,
const std::string &KernelName)
:
KernelNameType(KernelNameType),
KernelEntryPointDecl(KernelEntryPointDecl),
KernelName(KernelName)
{}
// FIXME: Added full enum to match the library implementation.
// Why does kind_first and kind_last exist?
enum kernel_param_kind_t {
kind_first,
kind_accessor = kind_first,
kind_std_layout,
kind_sampler,
kind_pointer,
kind_specialization_constants_buffer,
kind_stream,
kind_last = kind_stream
};
Comment on lines +26 to +35
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation for defining kernel_param_kind_t as a member of SYCLKernelInfo? I think it makes sense for KernelParamDesc to be a member since it is just implementation detail, but since the enumeration is exposed as part of the interface, I think it would be more ergonomic to define it at namespace scope.

The enumeration name and enumerators should be renamed to follow LLVM style. Perhaps SYCLKernelParamKind with the enumerators names SKPK_* (see PrimitiveCopyKind and DestructionKind as precedents). Alternatively, use of enum class would allow omitting the SKPK_ prefix (see NonConstantStorageReason and AutoTypeKeyword as precedents).

Since this enumeration is expected to match a definition in the SYCL run-time, it would be good to define an explicit underlying type for it; perhaps just int8_t.

If we don't have a use for kind_first and kind_last, they can be omitted for now.


public:
SYCLKernelInfo(CanQualType KernelNameType,
const FunctionDecl *KernelEntryPointDecl,
const std::string &KernelName)
: KernelNameType(KernelNameType),
KernelEntryPointDecl(KernelEntryPointDecl), KernelName(KernelName) {}

CanQualType GetKernelNameType() const {
return KernelNameType;
Expand All @@ -43,10 +53,33 @@ class SYCLKernelInfo {
return KernelName;
}

size_t GetParamCount() const { return Params.size(); }

void addParamDesc(kernel_param_kind_t Kind, QualType Ty) {
KernelParamDesc PD;
PD.Kind = Kind;
PD.Type = Ty;
Params.push_back(PD);
}

const kernel_param_kind_t &GetParamKind(int i) const {
return Params[i].Kind;
}

const QualType &GetParamTy(int i) const { return Params[i].Type; }

private:
// Kernel caller function parameter descriptor.
struct KernelParamDesc {
kernel_param_kind_t Kind = kind_last;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind_last (equivalent to kind_stream here) makes for an odd default. If a default is needed, then I recommend introducing a new enumerator (e.g., kind_invalid) for that purpose. I'm generally inclined not to introduce such "invalid" values unless absolutely required though. In this case, it looks like a default can be avoided by adding a constructor that requires the kind to be provided (and thus eliding the default constructor; assuming SmallVector doesn't complain about the type not being default constructible; which it might).

Copy link
Collaborator Author

@elizabethandrews elizabethandrews Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have a good answer here. I just kept the implementation of the enum as it is in syclos.

QualType Type;
KernelParamDesc() = default;
};

CanQualType KernelNameType;
const FunctionDecl *KernelEntryPointDecl;
std::string KernelName;
SmallVector<KernelParamDesc, 8> Params;
};

} // namespace clang
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Basic/Builtins.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ enum LanguageID : uint16_t {
OCL_DSE = 0x400, // builtin requires OpenCL device side enqueue.
ALL_OCL_LANGUAGES = 0x800, // builtin for OCL languages.
HLSL_LANG = 0x1000, // builtin requires HLSL.
SYCL_LANG = 0x2000, // builtin requires SYCL.
ALL_LANGUAGES = C_LANG | CXX_LANG | OBJC_LANG, // builtin for all languages.
ALL_GNU_LANGUAGES = ALL_LANGUAGES | GNU_LANG, // builtin requires GNU mode.
ALL_MS_LANGUAGES = ALL_LANGUAGES | MS_LANG // builtin requires MS mode.
Expand Down
61 changes: 61 additions & 0 deletions clang/include/clang/Basic/Builtins.td
Original file line number Diff line number Diff line change
Expand Up @@ -4598,6 +4598,67 @@ def GetDeviceSideMangledName : LangBuiltin<"CUDA_LANG"> {
let Prototype = "char const*(...)";
}

// SYCL
def SYCLKernelName : LangBuiltin<"SYCL_LANG"> {
let Spellings = ["__builtin_sycl_kernel_name"];
let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
let Prototype = "char const*(...)";
}

def SYCLKernelParamCount : LangBuiltin<"SYCL_LANG"> {
let Spellings = ["__builtin_sycl_kernel_param_count"];
let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
let Prototype = "int(...)";
}

def SYCLKernelParamKind : LangBuiltin<"SYCL_LANG"> {
let Spellings = ["__builtin_sycl_kernel_param_kind"];
let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
let Prototype = "int(...)";
}

def SYCLKernelParamSize : LangBuiltin<"SYCL_LANG"> {
let Spellings = ["__builtin_sycl_kernel_param_size"];
let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
let Prototype = "int(...)";
}

def SYCLKernelParamOffset : LangBuiltin<"SYCL_LANG"> {
let Spellings = ["__builtin_sycl_kernel_param_offset"];
let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
let Prototype = "int(...)";
}

def SYCLKernelParamAccessTarget : LangBuiltin<"SYCL_LANG"> {
let Spellings = ["__builtin_sycl_kernel_param_access_target"];
let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
let Prototype = "int(...)";
}

def SYCLKernelFileName : LangBuiltin<"SYCL_LANG"> {
let Spellings = ["__builtin_sycl_kernel_file_name"];
let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
let Prototype = "char const*(...)";
}

def SYCLKernelFunctionName : LangBuiltin<"SYCL_LANG"> {
let Spellings = ["__builtin_sycl_kernel_function_name"];
let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
let Prototype = "char const*(...)";
}

def SYCLKernelLineNumber : LangBuiltin<"SYCL_LANG"> {
let Spellings = ["__builtin_sycl_kernel_line_number"];
let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
let Prototype = "unsigned int(...)";
}

def SYCLKernelColumnNumber : LangBuiltin<"SYCL_LANG"> {
let Spellings = ["__builtin_sycl_kernel_column_number"];
let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
let Prototype = "unsigned int(...)";
}

// HLSL
def HLSLAll : LangBuiltin<"HLSL_LANG"> {
let Spellings = ["__builtin_hlsl_elementwise_all"];
Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,9 @@ def warn_unreachable_association : Warning<
InGroup<UnreachableCodeGenericAssoc>;

/// Built-in functions.
def err_builtin_invalid_argument_count : Error<
"builtin %plural{0:takes no arguments|1:takes one argument|"
":requires exactly %0 arguments}0">;
def ext_implicit_lib_function_decl : ExtWarn<
"implicitly declaring library function '%0' with type %1">,
InGroup<ImplicitFunctionDeclare>;
Expand Down Expand Up @@ -12199,6 +12202,9 @@ def err_sycl_kernel_name_type : Error<
def err_sycl_kernel_name_conflict : Error<
"'sycl_kernel_entry_point' kernel name argument conflicts with a previous"
" declaration">;
def err_sycl_kernel_name_invalid_arg : Error<"invalid argument; expected a class "
"or structure with a member typedef "
"or type alias alias named 'type'">;
Comment on lines +12205 to +12207
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def err_sycl_kernel_name_invalid_arg : Error<"invalid argument; expected a class "
"or structure with a member typedef "
"or type alias alias named 'type'">;
def err_sycl_kernel_name_invalid_arg : Error<"invalid argument; expected a class "
"or structure with a member typedef "
"or type alias named 'type'">;


def warn_cuda_maxclusterrank_sm_90 : Warning<
"maxclusterrank requires sm_90 or higher, CUDA arch provided: %0, ignoring "
Expand Down
41 changes: 35 additions & 6 deletions clang/lib/AST/ASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11994,8 +11994,15 @@ bool ASTContext::DeclMustBeEmitted(const Decl *D) {

// SYCL kernel entry point functions are used to generate and emit
// the offload kernel.
if (LangOpts.SYCLIsDevice && FD->hasAttr<SYCLKernelEntryPointAttr>())
if (LangOpts.SYCLIsDevice) {
if (D->hasAttr<SYCLKernelEntryPointAttr>())
return true;
// FIXME: Existing tests fail if we limit emission to the kernel caller
// function and functions called from it. Once the sycl_device attribute
// is implemented, modify this check (and tests) to include it and
// return false.
Comment on lines +12000 to +12003
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since sycl_device doesn't exist upstream yet, I think we should refrain from mentioning it here. The following suggested change defers to the SYCL specification for expected future behavior.

I suspect more changes will be needed in this function. For example, should we be returning true for declarations attributed with AliasAttr or UsedAttr? Or for global variables with external linkage?

(I tend to prefix "FIXME" on every line of a FIXME comment so that a grep for FIXME yields the entire comment. It also helps to separate FIXME content from other content. Not everyone does this of course).

Suggested change
// FIXME: Existing tests fail if we limit emission to the kernel caller
// function and functions called from it. Once the sycl_device attribute
// is implemented, modify this check (and tests) to include it and
// return false.
// FIXME: The only functions that must be emitted during device compilation
// FIXME: are the kernel entry points and functions declared with SYCL_EXTERNAL.
// FIXME: However, some existing tests fail if the set of emitted functions is
// FIXME: limited to those functions and the ones they call; further investigation
// FIXME: is needed to determine how to address those test failures.

// return false;
}

// Constructors and destructors are required.
if (FD->hasAttr<ConstructorAttr>() || FD->hasAttr<DestructorAttr>())
Expand Down Expand Up @@ -13781,9 +13788,8 @@ void ASTContext::getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
}
}

static SYCLKernelInfo BuildSYCLKernelInfo(ASTContext &Context,
CanQualType KernelNameType,
const FunctionDecl *FD) {
static std::string GetSYCLKernelCallerName(ASTContext &Context,
CanQualType KernelNameType) {
// FIXME: Host and device compilations must agree on a name for the generated
// FIXME: SYCL kernel caller function. The name is provided to the SYCL
// FIXME: library on the host via __builtin_sycl_kernel_name() and the SYCL
Expand Down Expand Up @@ -13820,9 +13826,32 @@ static SYCLKernelInfo BuildSYCLKernelInfo(ASTContext &Context,
Buffer.reserve(128);
llvm::raw_string_ostream Out(Buffer);
MC->mangleSYCLKernelCallerName(KernelNameType, Out);
std::string KernelName = Out.str();
return Out.str();
}

static void CreateSYCLKernelParamDesc(ASTContext &Ctx, const FunctionDecl *FD,
SYCLKernelInfo &KernelInfo) {
if (FD->getNumParams() == 0)
return;

for (const ParmVarDecl *KernelParam : FD->parameters()) {
KernelInfo.addParamDesc(SYCLKernelInfo::kind_std_layout,
KernelParam->getType());
}
}

static SYCLKernelInfo BuildSYCLKernelInfo(ASTContext &Context,
CanQualType KernelNameType,
const FunctionDecl *FD) {
// Get the mangled name.
std::string KernelCallerName =
GetSYCLKernelCallerName(Context, KernelNameType);

SYCLKernelInfo KernelInfo{KernelNameType, FD, KernelCallerName};

CreateSYCLKernelParamDesc(Context, FD, KernelInfo);

return { KernelNameType, FD, KernelName };
return KernelInfo;
}

void ASTContext::registerSYCLEntryPointFunction(FunctionDecl *FD) {
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/Basic/Builtins.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ static bool builtinIsSupported(const Builtin::Info &BuiltinInfo,
/* CUDA Unsupported */
if (!LangOpts.CUDA && BuiltinInfo.Langs == CUDA_LANG)
return false;
/* SYCL Unsupported */
if (!LangOpts.isSYCL() && BuiltinInfo.Langs == SYCL_LANG)
return false;
/* CPlusPlus Unsupported */
if (!LangOpts.CPlusPlus && BuiltinInfo.Langs == CXX_LANG)
return false;
Expand Down
92 changes: 92 additions & 0 deletions clang/lib/CodeGen/CGBuiltin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "clang/Basic/TargetBuiltins.h"
#include "clang/Basic/TargetInfo.h"
#include "clang/Basic/TargetOptions.h"
#include "clang/Basic/IdentifierTable.h"
#include "clang/CodeGen/CGFunctionInfo.h"
#include "clang/Frontend/FrontendDiagnostic.h"
#include "llvm/ADT/APFloat.h"
Expand Down Expand Up @@ -2538,6 +2539,22 @@ static RValue EmitHipStdParUnsupportedBuiltin(CodeGenFunction *CGF,
return RValue::get(CGF->Builder.CreateCall(UBF, Args));
}

static const SYCLKernelInfo *GetSYCLKernelInfo(ASTContext &Ctx,
const CallExpr *E) {
// Argument to the builtin is a type trait which is used to retrieve the
// kernel name type.
// FIXME: Improve the comment.
RecordDecl *RD = E->getArg(0)->getType()->castAs<RecordType>()->getDecl();
IdentifierTable &IdentTable = Ctx.Idents;
auto Name = DeclarationName(&(IdentTable.get("type")));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IdentifierTable::get() will create and return an IdentifierInfo reference even if no such identifier as been encountered yet. Perhaps it would be better to use find() here and to assert the identifier is found.

Suggested change
auto Name = DeclarationName(&(IdentTable.get("type")));
auto TypeIdentInfo = IdentTable.find("type");
assert(TypeIdentInfo != IdentTable.end();
auto Name = DeclarationName(*TypeIdentInfo);

NamedDecl *ND = (RD->lookup(Name)).front();
TypedefNameDecl *TD = cast<TypedefNameDecl>(ND);
CanQualType KernelNameType = Ctx.getCanonicalType(TD->getUnderlyingType());

// Retrieve KernelInfo using the kernel name.
return Ctx.findSYCLKernelInfo(KernelNameType);
}

RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
const CallExpr *E,
ReturnValueSlot ReturnValue) {
Expand Down Expand Up @@ -5992,6 +6009,81 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
auto Str = CGM.GetAddrOfConstantCString(Name, "");
return RValue::get(Str.getPointer());
}
case Builtin::BI__builtin_sycl_kernel_name: {
// Retrieve the kernel info corresponding to kernel name type.
const SYCLKernelInfo *KernelInfo = GetSYCLKernelInfo(getContext(), E);
assert(KernelInfo && "Type does not correspond to a SYCL kernel name.");

// Emit the mangled name.
auto Str = CGM.GetAddrOfConstantCString(KernelInfo->GetKernelName(), "");
return RValue::get(Str.getPointer());
}
case Builtin::BI__builtin_sycl_kernel_param_count: {
// Retrieve the kernel info corresponding to kernel name type.
const SYCLKernelInfo *KernelInfo = GetSYCLKernelInfo(getContext(), E);
assert(KernelInfo && "Type does not correspond to a SYCL kernel name.");
return RValue::get(
llvm::ConstantInt::get(Int32Ty, KernelInfo->GetParamCount()));
Comment on lines +6025 to +6026
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The builtin signature specifies that it returns int. I think this should match.

Suggested change
return RValue::get(
llvm::ConstantInt::get(Int32Ty, KernelInfo->GetParamCount()));
return RValue::get(
llvm::ConstantInt::get(IntTy, KernelInfo->GetParamCount()));

}
case Builtin::BI__builtin_sycl_kernel_param_kind: {
// Retrieve the kernel info corresponding to kernel name type.
const SYCLKernelInfo *KernelInfo = GetSYCLKernelInfo(getContext(), E);
assert(KernelInfo && "Type does not correspond to a SYCL kernel name.");
// Emit total number of parameters of kernel caller function.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't match.

Suggested change
// Emit total number of parameters of kernel caller function.
// Emit the parameter kind for the requested parameter.

const Expr *ParamNoExpr = E->getArg(1);
Expr::EvalResult Result;
ParamNoExpr->EvaluateAsInt(Result, getContext());
unsigned ParamNo = Result.Val.getInt().getZExtValue();
Comment on lines +6034 to +6036
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is error checking needed here? Can the evaluation fail? What happens if the evaluation produces a negative number? It looks like that will get implicitly converted to an unsigned value that is likely to exceed the parameter count.

return RValue::get(
llvm::ConstantInt::get(Int32Ty, KernelInfo->GetParamKind(ParamNo)));
Comment on lines +6037 to +6038
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type here should match the underlying type of the kernel_param_kind_t enumeration. I think the builtin is declared to return an int.

Suggested change
return RValue::get(
llvm::ConstantInt::get(Int32Ty, KernelInfo->GetParamKind(ParamNo)));
return RValue::get(
llvm::ConstantInt::get(IntTy, KernelInfo->GetParamKind(ParamNo)));

}
case Builtin::BI__builtin_sycl_kernel_param_size: {
// Retrieve the kernel info corresponding to kernel name type.
const SYCLKernelInfo *KernelInfo = GetSYCLKernelInfo(getContext(), E);
assert(KernelInfo && "Type does not correspond to a SYCL kernel name.");
// Emit total number of parameters of kernel caller function.
const Expr *ParamNoExpr = E->getArg(1);
Expr::EvalResult Result;
ParamNoExpr->EvaluateAsInt(Result, getContext());
unsigned ParamNo = Result.Val.getInt().getZExtValue();
QualType ParamTy = KernelInfo->GetParamTy(ParamNo);
// FIXME: Add check to ensure complete type.
return RValue::get(llvm::ConstantInt::get(
Int32Ty, getContext().getTypeSizeInChars(ParamTy).getQuantity()));
}
case Builtin::BI__builtin_sycl_kernel_param_offset: {
// Retrieve the kernel info corresponding to kernel name type.
const SYCLKernelInfo *KernelInfo = GetSYCLKernelInfo(getContext(), E);
assert(KernelInfo && "Type does not correspond to a SYCL kernel name.");
// FIXME: Offset is used only when kernel object is decomposed to identify
// offset of field in kernel object. What should the offset be for
// additional non-kernel object parameters?
return RValue::get(llvm::ConstantInt::get(Int32Ty, 0));
}
case Builtin::BI__builtin_sycl_kernel_param_access_target: {
// FIXME: This is a dummy value. This builtin will be implemented when
// support for special sycl types is implemented.
return RValue::get(llvm::ConstantInt::get(Int32Ty, 0));
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the above builtins that take a parameter index as an argument, we'll need to support the parameter index argument being a non-constant; see how Sergey implemented getKernelParamDescs() in intel/llvm#15070. Code consuming the parameter information will look something like:

  int param_count = __builtin_sycl_kernel_param_count(KernelIdentity<KernelNameType>());
  for (int i = 0; i < param_count; ++i) {
    int param_size = __builtin_sycl_kernel_param_size(KernelIdentity<KernelNameType>(), i);
    ...
  }

There are a few implications of this.

  1. When the parameter index argument can be evaluated at compile-time, IR can be emitted to just provide the parameter info value as the code does now. However, we need to issue a diagnostic if the parameter index is not in range.
  2. When the parameter index argument can't be evaluated at compile-time, IR will have to be emitted to retrieve the value from a stored array of elements. This will presumably require emitting something like a static local constexpr variable (something with internal linkage) that the IR will index into. We can't issue a diagnostic for an out of range parameter index in this case of course, so the IR should include a comparison and branch to an unreachable instruction.

case Builtin::BI__builtin_sycl_kernel_file_name:
case Builtin::BI__builtin_sycl_kernel_function_name: {
// FIXME: This is a dummy value. These builtins provide information
// about the kernel object. In the new design, the we do not have
// special status for the kernel object, so it is unclear what these
// builtins should return, or if they even need to exist. Support will
// be added or removed after investigation.
auto Str = CGM.GetAddrOfConstantCString("DummyString", "");
return RValue::get(Str.getPointer());
}
case Builtin::BI__builtin_sycl_kernel_line_number:
case Builtin::BI__builtin_sycl_kernel_column_number: {
// FIXME: This is a dummy value. These builtins provide information
// about the kernel object. In the new design, the we do not have
// special status for the kernel object, so it is unclear what these
// builtins should return, or if they even need to exist. Support will
// be added or removed after investigation.
return RValue::get(llvm::ConstantInt::get(Int32Ty, 0));
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier comments apply to the above builtins as well. E.g., prefer IntTy over Int32Ty and error checking of a parameter index argument.

}

// If this is an alias for a lib function (e.g. __builtin_sin), emit
Expand Down
Loading