doc: LLVM mtune in 2025.10 Toolchain Only Supports RV32#60
Merged
Conversation
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 7de1efd in 1 minute and 32 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. source/toolchain/changelog.rst:53
- Draft comment:
Consider adding a space before the parenthesis for clarity (i.e., 'series (RV32 Only)'). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. source/toolchain/changelog.rst:53
- Draft comment:
Typographical suggestion: Consider adding a space before the opening parenthesis in "series(RV32 Only)" for better readability. For example, consider "series (RV32 Only)". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a very minor stylistic comment about spacing. While the suggestion might improve readability slightly, it's not a functional issue. The rules state "Do NOT make comments that are obvious or unimportant" and "Do NOT comment unless there is clearly a code change required." This is a documentation file (changelog.rst), and the spacing issue is extremely minor. The comment is technically correct - adding a space would follow common English typography conventions - but it's borderline trivial. However, for documentation, proper formatting can be important for professionalism. I need to weigh whether this rises to the level of "clearly a code change required" or if it's too minor. While proper spacing in documentation is good practice, this is an extremely minor stylistic issue that doesn't affect functionality or comprehension. The rules emphasize not making "obvious or unimportant" comments, and a single missing space in a changelog entry could be considered unimportant. However, this is a documentation file where readability and professional formatting matter. The comment is actionable with a clear suggestion, and proper spacing before parentheses is a standard typography convention. It's a quick fix that improves quality. This is a borderline case - it's a minor but valid stylistic improvement to documentation. Given the emphasis on not making unimportant comments and the trivial nature of a single space, I lean toward deleting this comment as it doesn't meet the threshold of "clearly a code change required."
Workflow ID: wflow_KhDEJn3eYY67bj06
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Member
|
不是这么写,应该如实写清楚,这里的mtune实现有哪些问题。 |
7de1efd to
756928e
Compare
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 756928e in 1 minute and 40 seconds. Click for details.
- Reviewed
35lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. source/toolchain/changelog.rst:53
- Draft comment:
Inconsistent architecture note: The '(RV32 Only)' tag was removed, but PR title indicates only RV32 is supported. Clarify if this change is correct. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This comment is explicitly asking to "Clarify if this change is correct" - this is a verification request to the PR author. According to the rules, I should NOT keep comments that ask the PR author to confirm their intention, explain, or double-check things. The comment doesn't point to a definite error; it's speculative based on information I cannot verify (the PR title). The author intentionally removed "(RV32 Only)", which suggests they meant to broaden the support or correct an inaccuracy. Without strong evidence that this removal is wrong, I should delete this comment. The comment might be catching a legitimate inconsistency if the PR title truly says "RV32 only" but the changelog now implies broader support. This could be a real documentation error that needs fixing. Even if there's a potential inconsistency with the PR title, the comment is asking for clarification rather than stating a definite problem. The rules explicitly say not to ask authors to verify or clarify. Additionally, I cannot see the PR title to verify the claim, and the rule states I should only keep comments with STRONG EVIDENCE of correctness. Without being able to verify the PR title claim, this is speculative. This comment should be deleted because it asks the PR author to clarify/verify the change rather than pointing to a definite error. It violates the rule against asking authors to confirm their intention or double-check things. Without access to the PR title to verify the claimed inconsistency, there's no strong evidence this comment is correct.
2. source/toolchain/llvm/intro.rst:159
- Draft comment:
The Known Issues section documents a bug with nuclei '-mtune' support. Ensure that details (e.g., impact on non-RV32 targets) are consistent with the changelog and PR title. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure consistency between the Known Issues section, changelog, and PR title. It is not making a specific code suggestion or pointing out a specific issue in the code. It is more of a general request for consistency, which falls under the rule of not asking the author to ensure things are consistent or verified.
Workflow ID: wflow_w6KWNOBRWSWowh7p
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Important
Adds documentation for a known issue in LLVM 2025.10 regarding Nuclei
-mtunesupport inintro.rst.intro.rstfor LLVM 2025.10.-mtunesupport where processor models useRISCVProcessorModelinstead ofRISCVTuneProcessorModel.This description was created by
for 756928e. You can customize this summary. It will automatically update as commits are pushed.