Skip to content

Conversation

xuhancn
Copy link
Collaborator

@xuhancn xuhancn commented Jun 8, 2024

Copy link

pytorch-bot bot commented Jun 8, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/128274

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ You can merge normally! (2 Unrelated Failures)

As of commit 521be4c with merge base 917387f (image):

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@xuhancn xuhancn requested a review from jgong5 June 8, 2024 01:24
@xuhancn xuhancn added the ciflow/trunk Trigger trunk jobs on your pull request label Jun 8, 2024
@xuhancn xuhancn marked this pull request as ready for review June 8, 2024 01:27
@xuhancn xuhancn added module: cpu CPU specific problem (e.g., perf, algorithm) intel priority matters to intel architecture from performance wise intel This tag is for PR from Intel labels Jun 8, 2024
@jgong5 jgong5 requested a review from desertfire June 8, 2024 02:11
@ezyang
Copy link
Contributor

ezyang commented Jun 8, 2024

Test? I don't understand what the change is doing, why are there falsey isa in the list of isa?

@xuhancn
Copy link
Collaborator Author

xuhancn commented Jun 8, 2024

Test? I don't understand what the change is doing, why are there falsey isa in the list of isa?

Hi @ezyang
I merge the new cpp_builder #125849 and miss the isa bool check. @jgong5 find the missing code, and then I submit this PR.
image

falsey fail are cuda related not caused by cpu isa.
I just testing whether re-run failed job can fix env related UT.

This PR reviewed by @jgong5 , don't worry.

@xuhancn
Copy link
Collaborator Author

xuhancn commented Jun 8, 2024

@pytorchbot rebase

@xuhancn
Copy link
Collaborator Author

xuhancn commented Jun 8, 2024

image

rebase to fix unrelated fail case.

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased xu_fix_miss_isa_bool_check onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout xu_fix_miss_isa_bool_check && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the xu_fix_miss_isa_bool_check branch from ea1e117 to 521be4c Compare June 8, 2024 16:45
@xuhancn
Copy link
Collaborator Author

xuhancn commented Jun 8, 2024

@ezyang previous fail was bringed from trunk, after rebase they are gone.

@ezyang
Copy link
Contributor

ezyang commented Jun 10, 2024

You asked me for a review, and now you're getting a review from me :P

I don't have anything fundamentally against this patch. But I would like to understand it more. That is what the code review process is for. I'll ask again:

  1. Under what circumstances can isa be falsey? How come it can be falsey but also be in the list of supported isas?
  2. This seems like a plausible bug to me. But how does one trigger this situation? Can we test it in CI? An explanation why it's not easy to test is also acceptable.

@xuhancn
Copy link
Collaborator Author

xuhancn commented Jun 10, 2024

You asked me for a review, and now you're getting a review from me :P

I don't have anything fundamentally against this patch. But I would like to understand it more. That is what the code review process is for. I'll ask again:

  1. Under what circumstances can isa be falsey? How come it can be falsey but also be in the list of supported isas?
  2. This seems like a plausible bug to me. But how does one trigger this situation? Can we test it in CI? An explanation why it's not easy to test is also acceptable.

Hi @ezyang
Previous PR lost and isa, it seems will lead to lost call class VecISA's def __bool__(self) -> bool:. This function will check compiler's capability for isa.

  1. If the compiler too old to support the checking isa, def __bool__(self) -> bool: will return False and skip the checking isa. Please check the compiler capability here: https://github.com/intel/intel-extension-for-pytorch/blob/main/docs/tutorials/features/isa_dynamic_dispatch.md#cpu-isa-build-compiler-requirement . If we use gcc 7.x for AVX512, this version gcc can't support AVX512, but we miss the checking and fail to catch the compiler error.
  2. Current CI at least using gcc 9, it can cover all testing isa. So we can't find my missing code: and isa.

@jgong5 tell me it is a serious bug, I need fix it ASAP before branch cut.

@xuhancn
Copy link
Collaborator Author

xuhancn commented Jun 10, 2024

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@xuhancn xuhancn deleted the xu_fix_miss_isa_bool_check branch June 10, 2024 02:46
TharinduRusira pushed a commit to TharinduRusira/pytorch that referenced this pull request Jun 14, 2024
New cpp builder missed ISA bool(dry-compile) check.
<img width="941" alt="image" src="https://github.com/pytorch/pytorch/assets/8433590/695ce911-7f6d-401d-b96b-2b9bda751b15">
@jgong5 Found this missing and then I submit this PR to fix it.

Pull Request resolved: pytorch#128274
Approved by: https://github.com/jgong5, https://github.com/ezyang
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request intel priority matters to intel architecture from performance wise intel This tag is for PR from Intel Merged module: cpu CPU specific problem (e.g., perf, algorithm) module: inductor open source topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants