Skip to content

[SYCL] Define SYCL_DISABLE_FALLBACK_ASSERT macro #4824

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.

Sign up for GitHub

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 1 commit into from
Nov 30, 2021
Merged

[SYCL] Define SYCL_DISABLE_FALLBACK_ASSERT macro #4824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion clang/lib/Frontend/InitPreprocessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1198,8 +1198,10 @@ static void InitializePredefinedMacros(const TargetInfo &TI,
DeviceSubArch != llvm::Triple::SPIRSubArch_fpga)
Builder.defineMacro("SYCL_USE_NATIVE_FP_ATOMICS");
// Enable generation of USM address spaces for FPGA.
if (DeviceSubArch == llvm::Triple::SPIRSubArch_fpga)
if (DeviceSubArch == llvm::Triple::SPIRSubArch_fpga) {
Builder.defineMacro("__ENABLE_USM_ADDR_SPACE__");
Builder.defineMacro("SYCL_DISABLE_FALLBACK_ASSERT");
Copy link
Contributor

Choose a reason for hiding this comment

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

Tagging @s-kanaev.
There is similar patch disabling fallback assert for all targets by default - #4694.
If the plan is to merge #4694, I think we can avoid defining this macro for FPGA target.
@s-kanaev, am I right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless we want a case where we enable fallback assert by default but keep it disabled for FPGA target, I assume we don't need this macro.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fallback assert is disabled by default due high impact issues being reported recently. Still, there's a plan to enable assert back. FPGA is an exception here as having one more kernel in device image impacts them more than GPU or CPU.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Thank you for clarifying. @bader, I think that means we want to keep the macro? At the moment, this will have no effect but once fallback assert is enabled, the macro will keep it disables for FPGA

Copy link
Contributor

Choose a reason for hiding this comment

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

#4694 renames macro.
I don't know if it make sense to proceed with this patch. It's not clear when fallback asserts will be enabled by default and whether this patch will work. According to my understanding, "enabling back" won't be as simple as just revert #4694.

Copy link
Contributor

Choose a reason for hiding this comment

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

@s-kanaev, what is overall plan for fallback assert? Are we going to commit #4824 and close #4694?

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely, #4694 is going to be closed. #4824 and #4780 are going to be committed. There were two types of issues:

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the second issue impacts non-FPGA devices as well. AFAIK, additional assert-specific device code is emitted for all devices even though assert is not used. Is it right? If so, what is the plan to address the overhead associated with the additional code?

Copy link
Contributor

@s-kanaev s-kanaev Nov 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

AFAIK, additional assert-specific device code is emitted for all devices even though assert is not used. Is it right?

Yes.
To overcome this impact I need to do some refactoring which is a bit stuck at limitations of sycl-post-link tool.
The limitation could be handled with employing of __sycl_kernel attribute, though, it requires for the function to be template which isn't acceptable at the moment. Also, @AlexeySachkov can't approve the w/a in sycl-post-link I presented to him.
The overall idea of refactoring can be caught with changes of devicelib in #4987.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, follow https://chris.beams.io/posts/git-commit/ and add proper PR title and description to #4987.

}
}
if (LangOpts.SYCLUnnamedLambda)
Builder.defineMacro("__SYCL_UNNAMED_LAMBDA__");
Expand Down
13 changes: 13 additions & 0 deletions clang/test/Preprocessor/sycl-macro-target-specific.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,16 @@
// RUN: | FileCheck --check-prefix=CHECK-USM-ADDR-SPACE-NEG %s
// CHECK-USM-ADDR-SPACE: #define __ENABLE_USM_ADDR_SPACE__
// CHECK-USM-ADDR-SPACE-NEG-NOT: #define __ENABLE_USM_ADDR_SPACE__

// RUN: %clang_cc1 %s -fsycl-is-device -triple spir64_fpga-unknown-unknown -E -dM \
// RUN: | FileCheck --check-prefix=CHECK-DISABLE-FALLBACK-ASSERT %s
// RUN: %clang_cc1 %s -fsycl-is-device -triple spir64-unknown-unknown -E -dM \
// RUN: | FileCheck --check-prefix=CHECK-DISABLE-FALLBACK-ASSERT-NEG %s
// RUN: %clang_cc1 %s -fsycl-is-device -triple spir64_gen-unknown-unknown -E -dM \
// RUN: | FileCheck --check-prefix=CHECK-DISABLE-FALLBACK-ASSERT-NEG %s
// RUN: %clang_cc1 %s -fsycl-is-device -triple spir64_x86_64-unknown-unknown -E -dM \
// RUN: | FileCheck --check-prefix=CHECK-DISABLE-FALLBACK-ASSERT-NEG %s
// RUN: %clang_cc1 %s -fsycl-is-device -triple nvptx64-nvidia-nvcl -E -dM \
// RUN: | FileCheck --check-prefix=CHECK-DISABLE-FALLBACK-ASSERT-NEG %s
// CHECK-DISABLE-FALLBACK-ASSERT: #define SYCL_DISABLE_FALLBACK_ASSERT
// CHECK-DISABLE-FALLBACK-ASSERT-NEG-NOT: #define SYCL_DISABLE_FALLBACK_ASSERT