Skip to content

[SYCL][Driver] Add deprecated warning for backend GRF options on PVC #12029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 1, 2023
Merged
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
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticDriverKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,9 @@ def err_drv_fsycl_wrong_optimization_options : Error<
def warn_drv_fsycl_add_default_spec_consts_image_flag_in_non_AOT : Warning<
"-fsycl-add-default-spec-consts-image flag has an effect only in Ahead of Time Compilation mode (AOT).">,
InGroup<SyclTarget>;
def warn_drv_ftarget_register_alloc_mode_pvc : Warning<
"using '%0' to set GRF mode on PVC hardware is deprecated; use '-ftarget-register-alloc-mode=pvc:%1'">,
InGroup<Deprecated>;
def err_drv_multiple_target_with_forced_target : Error<
"multiple target usage with '%0' is not supported with '%1'">;
def err_drv_failed_to_deduce_target_from_arch : Error<
Expand Down
39 changes: 33 additions & 6 deletions clang/lib/Driver/ToolChains/SYCL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -781,12 +781,15 @@ void SYCL::fpga::BackendCompiler::ConstructJob(
C.addCommand(std::move(Cmd));
}

static llvm::StringMap<StringRef> GRFModeFlagMap{
{"auto", "-ze-intel-enable-auto-large-GRF-mode"},
{"small", "-ze-intel-128-GRF-per-thread"},
{"large", "-ze-opt-large-register-file"}};

StringRef SYCL::gen::getGenGRFFlag(StringRef GRFMode) {
return llvm::StringSwitch<StringRef>(GRFMode)
.Case("auto", "-ze-intel-enable-auto-large-GRF-mode")
.Case("small", "-ze-intel-128-GRF-per-thread")
.Case("large", "-ze-opt-large-register-file")
.Default("");
if (!GRFModeFlagMap.contains(GRFMode))
return "";
return GRFModeFlagMap[GRFMode];
}

void SYCL::gen::BackendCompiler::ConstructJob(Compilation &C,
Expand Down Expand Up @@ -1100,6 +1103,26 @@ void SYCLToolChain::TranslateGPUTargetOpt(const llvm::opt::ArgList &Args,
}
}

static void WarnForDeprecatedBackendOpts(const Driver &D,
const llvm::Triple &Triple,
StringRef Device, StringRef ArgString,
const llvm::opt::Arg *A) {
// Suggest users passing GRF backend opts on PVC to use
// -ftarget-register-alloc-mode and

if (!ArgString.contains("-device pvc") && !Device.contains("pvc"))
return;
// Make sure to only warn for once for gen targets as the translate
// options tree is called twice but only the second time has the
// device set.
if (Triple.isSPIR() && Triple.getSubArch() == llvm::Triple::SPIRSubArch_gen &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kinda gross but we end up calling the entire translate backend opt tree super early in selectBfloatLibs so if we don't do anything the warning is thrown twice for the same backend opt instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

this would probably need some kind of cleanup so we can do BE arg checking from anywhere and not be impacted by multiple diagnostic outputs, but yeah, it's not pretty.

!A->isClaimed())
return;
for (const auto &[Mode, Flag] : GRFModeFlagMap)
if (ArgString.contains(Flag))
D.Diag(diag::warn_drv_ftarget_register_alloc_mode_pvc) << Flag << Mode;
}

// Expects a specific type of option (e.g. -Xsycl-target-backend) and will
// extract the arguments.
void SYCLToolChain::TranslateTargetOpt(const llvm::opt::ArgList &Args,
Expand Down Expand Up @@ -1143,7 +1166,8 @@ void SYCLToolChain::TranslateTargetOpt(const llvm::opt::ArgList &Args,
} else
// Triple found, add the next argument in line.
ArgString = A->getValue(1);

WarnForDeprecatedBackendOpts(getDriver(), getTriple(), Device, ArgString,
A);
parseTargetOpts(ArgString, Args, CmdArgs);
A->claim();
}
Expand Down Expand Up @@ -1313,12 +1337,15 @@ void SYCLToolChain::TranslateBackendTargetArgs(
if (A->getOption().matches(options::OPT_Xs)) {
// Take the arg and create an option out of it.
CmdArgs.push_back(Args.MakeArgString(Twine("-") + A->getValue()));
WarnForDeprecatedBackendOpts(getDriver(), Triple, Device, A->getValue(),
A);
A->claim();
continue;
}
if (A->getOption().matches(options::OPT_Xs_separate)) {
StringRef ArgString(A->getValue());
parseTargetOpts(ArgString, Args, CmdArgs);
WarnForDeprecatedBackendOpts(getDriver(), Triple, Device, ArgString, A);
A->claim();
continue;
}
Expand Down
22 changes: 22 additions & 0 deletions clang/test/Driver/sycl-ftarget-register-alloc-mode-warning.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Test warning SYCL -ftarget-register-alloc-mode for pvc

// RUN: %clang -fsycl -Xs "-device pvc -ze-opt-large-register-file" -### %s 2>&1 | FileCheck -check-prefix=LARGE %s
// RUN: %clang -fsycl -Xs "-device pvc -ze-intel-128-GRF-per-thread" -### %s 2>&1 | FileCheck -check-prefix=SMALL %s
// RUN: %clang -fsycl -Xs "-ze-intel-enable-auto-large-GRF-mode -device pvc" -### %s 2>&1 | FileCheck -check-prefix=AUTO %s

// RUN: %clang -fsycl -fsycl-targets=intel_gpu_pvc -Xs "-ze-opt-large-register-file" -### %s 2>&1 | FileCheck -check-prefix=LARGE %s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did I miss any important cases here?

Copy link
Contributor

Choose a reason for hiding this comment

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

So do we care about potential usages like -Xs -ze-opt-large-register-file -Xs "-device pvc"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im gonna go with no because i've never seen anybody do that and it seems like it would be pretty annoying to deal with


// RUN: %clang -fsycl -fsycl-targets=intel_gpu_pvc -Xs"-ze-opt-large-register-file" -### %s 2>&1 | FileCheck -check-prefix=LARGE %s

// RUN: %clang -fsycl -Xsycl-target-backend=spir64 "-device pvc -ze-intel-128-GRF-per-thread" -### %s 2>&1 | FileCheck -check-prefix=SMALL %s

// RUN: %clang -fsycl -fsycl-targets=intel_gpu_pvc -Xsycl-target-backend "-ze-opt-large-register-file" -### %s 2>&1 | FileCheck -check-prefix=LARGE %s

// RUN: %clang -fsycl -fsycl-targets=spir64_gen -Xs "-ze-opt-large-register-file -device pvc" -### %s 2>&1 | FileCheck -check-prefix=LARGE %s

// RUN: %clang -fsycl -fsycl-targets=spir64,spir64_gen -Xsycl-target-backend=spir64 "-ze-opt-large-register-file -device pvc" \
// RUN: -Xsycl-target-backend=spir64_gen "-ze-intel-enable-auto-large-GRF-mode -device pvc" -### %s 2>&1 | FileCheck -check-prefixes=LARGE,AUTO %s

// LARGE: warning: using '-ze-opt-large-register-file' to set GRF mode on PVC hardware is deprecated; use '-ftarget-register-alloc-mode=pvc:large'
// SMALL: warning: using '-ze-intel-128-GRF-per-thread' to set GRF mode on PVC hardware is deprecated; use '-ftarget-register-alloc-mode=pvc:small'
// AUTO: warning: using '-ze-intel-enable-auto-large-GRF-mode' to set GRF mode on PVC hardware is deprecated; use '-ftarget-register-alloc-mode=pvc:auto'