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

Increase strictness of global isel use for ROCM #19247

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tpopp
Copy link
Contributor

@tpopp tpopp commented Nov 21, 2024

Increase strictness while this is primarily for development instead of robust production cases (though it is still quite robust).

This will prevent fallback from global-isel with unsupported inputs to prevent a situation of attributing a witnessed change to improved global isel results that are actually caused by not using global isel or to prevent a lack of change caused by not actually utilizing global isel.

In this form, unsupported inputs will fail to compile entirely.

@tpopp tpopp requested a review from kuhar as a code owner November 21, 2024 16:08
Increase strictness while this is primarily for development instead of
robust production cases (though it is still quite robust).

This will prevent fallback from global-isel with unsupported inputs
to prevent a situation of attributing a witnessed change to improved
global isel results that are actually caused by not using global isel
or to prevent a lack of change caused by not actually utilizing global
isel.

In this form, unsupported inputs will fail to compile entirely.
@tpopp tpopp force-pushed the users/tpopp/global-isel-caution branch from 10ad800 to 41967aa Compare November 21, 2024 16:11
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

I imagine this cannot be easily tested, right?

Looks good, just one suggestion.

@@ -462,7 +462,12 @@ class ROCMTargetBackend final : public TargetBackend {
opt.UnsafeFPMath = false;
opt.NoInfsFPMath = false;
opt.NoNaNsFPMath = true;
// Be extra cautious while this is less tested, and prevent unknown
// fallbacks from global isel.
Copy link
Member

Choose a reason for hiding this comment

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

Could you briefly explain what this option does and how we can observe the difference?

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.

2 participants