Skip to content

Remove validateMinimumArch#173

Merged
zasdfgbnm merged 4 commits intomainfrom
disable-arch-validate
Apr 17, 2023
Merged

Remove validateMinimumArch#173
zasdfgbnm merged 4 commits intomainfrom
disable-arch-validate

Conversation

@zasdfgbnm
Copy link
Collaborator

I don't think these checks make sense. I don't see any reason for disallow lowering of an Ampere kernel on Volta, or on a machine without GPU. Cross compilation support is common for compilers, for example, nvcc has -arch to specify the target architecture, even if your machine has no GPU at all.

This is blocking #155, because the bank conflict checking test fails on Volta. I don't think these failure makes sense.

@zasdfgbnm zasdfgbnm requested a review from naoyam April 17, 2023 15:40
@zasdfgbnm
Copy link
Collaborator Author

!build

//! higher than provided major.minor.
void validateMinimumArch(int major, int minor) {
// Skip checking arch if disabled.
if (isOptionDisabled(DisableOption::ArchCheck)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this option still used?

Copy link
Collaborator Author

@zasdfgbnm zasdfgbnm Apr 17, 2023

Choose a reason for hiding this comment

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

Yes, in executor_utils.cpp getCompiledKernel, there is a

  TORCH_CHECK(
      !isOptionDisabled(DisableOption::ArchCheck),
      "NVFuser Compile: arch check disabled, should not return any compiled kernel");

don't think this makes much sense either.... Let me remove it.

@zasdfgbnm
Copy link
Collaborator Author

!build

Copy link
Collaborator

@naoyam naoyam left a comment

Choose a reason for hiding this comment

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

LGTM

@zasdfgbnm zasdfgbnm merged commit d89c136 into main Apr 17, 2023
@zasdfgbnm zasdfgbnm deleted the disable-arch-validate branch April 17, 2023 17:16
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