[InlineAsm] refactor InlineAsm class NFC #65649
Merged
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
I would like to steal one of these bits to denote whether a kind may be spilled by the register allocator or not, but I'm afraid to touch of any this code using bitwise operands. Make flags a first class type using bitfields, rather than launder data around via `unsigned`.
nickdesaulniers
requested review from
jyknight,
isanbard,
bwendling,
qcolombet,
efriedma-quic and
echristo
nickdesaulniers
commented
Sep 7, 2023
bwendling
reviewed
Sep 7, 2023
jyknight
reviewed
Sep 7, 2023
| @@ -241,6 +225,7 @@ class InlineAsm final : public Value { | |||
| // Addresses are included here as they need to be treated the same by the | |||
| // backend, the only difference is that they are not used to actaully | |||
| // access memory by the instruction. | |||
| // TODO: convert to enum? | |||
There was a problem hiding this comment.
If it's ok with you, I'd prefer to do this in a distinct PR, akin to:
commit 2fad6e6 ("[InlineAsm] wrap Kind in enum class NFC")
oh yeah! done. Huh, I miss |
|
Ok, thanks all for the reviews+comments+feedback thus far. I think I've addressed everything; this should be ready for another round of reviews. |
cor3ntin
added
llvm:SelectionDAG
qcolombet
approved these changes
Sep 11, 2023
jyknight
approved these changes
Sep 11, 2023
There was a problem hiding this comment.
Just a sidenote: multiple overlapping Bitfield::Element could be used for "Data", if desired. I don't think it matters here, so approving as is. But, it would be useful if we wanted to add additional fields for only some of the options later (e.g. Mem Constraint code doesn't need as many bits as it's given, so other data could fit next to it in that case).
Oh right, good idea. Ok, I will clean that up too in follow up PRs. |
|
This change appears to have broken the MLIR Windows buildbot (and probably some other Windows bots). |
nickdesaulniers
added a commit
that referenced
this pull request
Sep 11, 2023
|
@NathanielMcVicar thanks for the report. Should be fixed as of |
|
Appears to be fixed (now there's another break, but much later)! Thanks so much for the quick resolution. |
nickdesaulniers
added a commit
to nickdesaulniers/llvm-project
that referenced
this pull request
Sep 13, 2023
Similar to commit 2fad6e6 ("[InlineAsm] wrap Kind in enum class NFC") Fix the TODOs added in commit 93bd428 ("[InlineAsm] refactor InlineAsm class NFC (llvm#65649)")
nickdesaulniers
added a commit
that referenced
this pull request
Sep 13, 2023
nickdesaulniers
added a commit
to nickdesaulniers/llvm-project
that referenced
this pull request
Sep 13, 2023
Use multiple Bitfield::Element's to provide more meaningful access to the same slice of bits. "Data" used in different contexts is confusing. Try to be as clear as possible, and in that vein, update the comment block for this class, too. This was suggested by @bwendling in llvm#65649 (comment) and @jyknight in llvm#65649 (review)
nickdesaulniers
added a commit
that referenced
this pull request
Sep 15, 2023
…#66297) Use multiple Bitfield::Element's to provide more meaningful access to the same slice of bits. "Data" used in different contexts is confusing. Try to be as clear as possible, and in that vein, update the comment block for this class, too. This was suggested by @bwendling in #65649 (comment) and @jyknight in #65649 (review)
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
I would like to steal one of these bits to denote whether a kind may be spilled by the register allocator or not, but I'm afraid to touch of any this code using bitwise operands. Make flags a first class type using bitfields, rather than launder data around via `unsigned`.
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
Similar to commit 2fad6e6 ("[InlineAsm] wrap Kind in enum class NFC") Fix the TODOs added in commit 93bd428 ("[InlineAsm] refactor InlineAsm class NFC (llvm#65649)")
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
…llvm#66297) Use multiple Bitfield::Element's to provide more meaningful access to the same slice of bits. "Data" used in different contexts is confusing. Try to be as clear as possible, and in that vein, update the comment block for this class, too. This was suggested by @bwendling in llvm#65649 (comment) and @jyknight in llvm#65649 (review)
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.
I would like to steal one of these bits to denote whether a kind may be
spilled by the register allocator or not, but I'm afraid to touch of any
this code using bitwise operands.
Make flags a first class type using bitfields, rather than launder data
around via
unsigned.