[TargetInstrInfo][NFC] Don't restrict isAddImmediate description to physical registers #72357
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
…hysical registers None of the in-tree implementations have different behaviour for physical vs virtual registers, and it seems would work equally well if used with virtual registers. As such, perhaps it's simplest to just drop that part of the doc comment.
djtodoro
approved these changes
Nov 16, 2023
|
LGTM. |
asb
added a commit
that referenced
this pull request
Nov 16, 2023
This hook is called by the target-independent implementation of TargetInstrInfo::describeLoadedValue. I've opted to test it via a C++ unit test, which although fiddly to set up seems the right way to test a function with such clear intended semantics (rather than testing the impact indirectly). isAddImmediate will never recognise ADDIW as an add immediate which I _think_ is conservatively correct, as the caller may not understand its semantics vs ADDI. Note that although the doc comment for isAddImmediate specifies its behaviour solely in terms of physical registers, none of the current in-tree implementations (including this one) bail out on virtual registers (see #72357).
sr-tream
pushed a commit
to sr-tream/llvm-project
that referenced
this pull request
Nov 20, 2023
This hook is called by the target-independent implementation of TargetInstrInfo::describeLoadedValue. I've opted to test it via a C++ unit test, which although fiddly to set up seems the right way to test a function with such clear intended semantics (rather than testing the impact indirectly). isAddImmediate will never recognise ADDIW as an add immediate which I _think_ is conservatively correct, as the caller may not understand its semantics vs ADDI. Note that although the doc comment for isAddImmediate specifies its behaviour solely in terms of physical registers, none of the current in-tree implementations (including this one) bail out on virtual registers (see llvm#72357).
sr-tream
pushed a commit
to sr-tream/llvm-project
that referenced
this pull request
Nov 20, 2023
…hysical registers (llvm#72357) None of the in-tree implementations have different behaviour for physical vs virtual registers, and it seems would work equally well if used with virtual registers. As such, perhaps it's simplest to just drop that part of the doc comment.
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
This hook is called by the target-independent implementation of TargetInstrInfo::describeLoadedValue. I've opted to test it via a C++ unit test, which although fiddly to set up seems the right way to test a function with such clear intended semantics (rather than testing the impact indirectly). isAddImmediate will never recognise ADDIW as an add immediate which I _think_ is conservatively correct, as the caller may not understand its semantics vs ADDI. Note that although the doc comment for isAddImmediate specifies its behaviour solely in terms of physical registers, none of the current in-tree implementations (including this one) bail out on virtual registers (see llvm#72357).
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
…hysical registers (llvm#72357) None of the in-tree implementations have different behaviour for physical vs virtual registers, and it seems would work equally well if used with virtual registers. As such, perhaps it's simplest to just drop that part of the doc comment.
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.
None of the in-tree implementations have different behaviour for physical vs virtual registers, and it seems would work equally well if used with virtual registers. As such, perhaps it's simplest to just drop that part of the doc comment.