NvvmArch:all_target_features fixes
#306
Merged
+298
−293
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Improve a few comments. - Combine three `nvvm_arch_target_feature_format_*` tests into one. - Avoid some unnecessary local variables. - Rename `nvvm_arch_all_target_features_includes_lower_capabilities`, and order things more sensibly within it. - Remove `target_feature_synthesis_supports_conditional_compilation_patterns` and `target_feature_synthesis_enables_correct_cfg_patterns` and `nvvm_arch_a_suffix_includes_all_available_instructions`. They are all testing the same things as `nvvm_arch_all_target_features`. (A couple of things from them were moved into `nvvm_arch_all_target_features`, to preserve test coverage.) This all just makes things more streamlined and avoids repetition, to get ready for subsequent changes.
Currently some `all_target_features` results are compared against a `Vec` literal, while some are only tested with a smattering of `contains`/`!contains` calls. This commit changes the latter one to use `Vec` literals. This is more thorough, easier to read, and demonstrates two existing bugs with `all_target_features`: - The values for the 'f' suffix ones are badly wrong. - The sort order is lexicographic, which puts `compute_100` before `compute_90`. The next commit will fix these bugs.
nnethercote
force-pushed
the
NvvmArch-fixes
branch
from
edee386 to
60ea9e7
Compare
It now does a single filter pass over the enum variants, which is simpler and fixes the sorting issue and the incorrect 'f' suffix results. I removed some comments in the `nvvm_arch_all_target_features` test, because they were low-value. There are now better comments within `all_target_features` that explain what's happening. I also remove the comment about PTX forward-compatibility. It was correct but confusing. This function answers the question "what features are available if I'm targeting a particular NvvmArch?" (backwards compatibility). That comment explained "what GPU CCs will this run on?" (forward compatibility). Also update the relevant section in the guide, where the 'f' details were incorrect. And make the terminology more consistent.
Several of them sort or inspect their vectors unnecessarily. We can just do `Vec` literal equality tests.
It's standard to have `use super::*;` in a test module, and this lets us remove a bunch of existing `use` items.
nnethercote
force-pushed
the
NvvmArch-fixes
branch
from
60ea9e7 to
2b9cf81
Compare
Collaborator
Author
|
Like in #305, "no space left on device" for the x86-64/Linux runs. Weird. |
Draft
LegNeato
approved these changes
Nov 19, 2025
Contributor
LegNeato
left a comment
LegNeato
left a comment
There was a problem hiding this comment.
Ok, I think you convinced me this is correct.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NvvmArch:all_target_featureshas some bugs. This PR fixes them. Details in the individual commits.