Skip to content
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

[SYCL] Enable SPV_INTEL_fp_max_error by default #16942

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

MrSidims
Copy link
Contributor

@MrSidims MrSidims commented Feb 10, 2025

Since a pass that cleans up unnecessary llvm.fpbuiltin intrinsics was removed from
#16107 we need to enable the extension by default to
successfully translate them.

@MrSidims MrSidims requested review from a team as code owners February 10, 2025 12:14
Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

LGTM. Just one question, why was the pass removed?

@MrSidims
Copy link
Contributor Author

MrSidims commented Feb 10, 2025

LGTM. Just one question, why was the pass removed?

The reason we originally added it in the first place is the following: clang now generated llvm.fpbuiltin.fdiv/sqrt by default (with 2.5 and 3.0 ULP precision). So the llvm-spirv translator would generate brand new decorations for them by default for every SYCL code that uses div or sqrt operations, making it impossible to use old drivers. So the idea behind the pass was: if LLVM IR module contains only llvm.fpbuiltin.fdiv/sqrt with 2.5 and 3.0 max error attributes, without any fdiv or llvm.sqrt instructions/intrinsics or without any other llvm.fpbuiltin.* intrinsics or without any llvm.fpbuiltin.fdiv/sqrt with other precision (aka default case) - then it means, that we would pass non-precise option to devices backend anyway and on LLVM IR level we can replace llvm.fpbuiltin.fdiv/sqrt with just fdiv and llvm.sqrt preventing generation of new decorations.

On the one hand it seem to be a nice idea, on the other hand oneAPI documentation says: please install the latest GPU drivers on your system, when install oneAPI version xx.yy.zz. So the extension should be implicitly supported by the user's system drivers (until we change oneAPI release criteria) and the pass is not needed from this perspective. And at the same time such pass, that iterates over all intrinsic declarations, analyses their usages and conditionally replaces calls to them is affecting compilation time.

IF we see a case, when OLD drivers must be used - the pass will be restored.

@MrSidims MrSidims changed the title [SYCL] Enable SPV_INTEL_fp_max_error by default for all devices but FPGA [SYCL] Enable SPV_INTEL_fp_max_error by default Feb 10, 2025
@MrSidims
Copy link
Contributor Author

@intel/dpcpp-clang-driver-reviewers @intel/dpcpp-cfe-reviewers please take a look

Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@MrSidims
Copy link
Contributor Author

MrSidims commented Feb 11, 2025

@intel/dpcpp-clang-driver-reviewers @intel/dpcpp-cfe-reviewers friendly ping, there are several tests failing without this patch

Copy link
Contributor

@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

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

OK for driver

Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

I'm not sure why FE review was required for this since the changes are to the driver and tools (I think) has other code owners. In any case this looks ok to me.

@MrSidims MrSidims requested a review from a team February 11, 2025 23:41
@MrSidims
Copy link
Contributor Author

@intel/llvm-gatekeepers please help with merge

@uditagarwal97 uditagarwal97 merged commit 964f963 into intel:sycl Feb 12, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants