Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[TableGen] DecoderEmitter clean-ups and modernization. #84832

Merged
merged 2 commits into from
Mar 12, 2024
Merged

[TableGen] DecoderEmitter clean-ups and modernization. #84832

merged 2 commits into from
Mar 12, 2024

Conversation

nvjle
Copy link
Contributor

@nvjle nvjle commented Mar 11, 2024 •
edited

The decoder emitter is showing some signs of age. This patch makes a few kinds of clean-ups:

  • Use ranged-for more widely, including using enumerate() for those loops maintaining a loop index along with the items.
  • Reduce the number of arguments to fieldFromInsn (removes an out reference parameter: CodingStandards). The insn_t argument to insnWithID can/should probably be removed soon too since modern C++ allows us to return a local container without a copy.
  • Use raw strings for the large emitted code segments. This enhances both readability and modifiability.

@nvjle
[TableGen] DecoderEmitter clean-ups and modernization.
d79bad2 
The decoder emitter is showing some signs of age. This patch makes few
kinds of clean-ups:
- Use ranged-for more widely, including using enumerate() for those
  loops maintaining a loop index along with the items.
- Reduce the number of arguments to fieldFromInsn (removes an out reference
  parameter: CodingStandards). The insn_t argument can/should probably be
  removed soon too since modern C++ allows us to return a local container
  without a copy.
- Use raw strings for the large emitted code segments. This enhances both
  readability and modifiability.
@nvjle
Copy link
Contributor Author

nvjle commented Mar 11, 2024 •
edited

If one of the selected reviewers would kindly review this patch, I'd appreciate it.

While this is technically NFC, I'd just like to be sure others agree with my use of more modern raw strings for the large emitted code fragments. This makes them far more readable (and modifiable) than the old-school strings-- since they look just like ordinary code (no quoting needed, no per-line stream operators, etc).

Also, in addition to make check, I've diffed the {RISCV,M68k}GenDisassemblerTable.inc before and after the patch for character-for-character identical results-- RISCV for a fixed-length decoder, and M68k for a variable-length decoder.

wangpc-pp
wangpc-pp approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it LGTM, but I don't know if this kind of rewrites is encouraged.
cc @topperc

jyknight
jyknight reviewed Mar 12, 2024
View reviewed changes
Copy link
Member

@jyknight jyknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good cleanup, just some minor comments.

One thing I've wanted to do for a long time, but never got around to:
I think we should move the long blobs of helper functions out of this file, and into a separate header which gets included from the output.

I don't think there's any real value in having them be emitted textually into the output code, vs included, and it'll be a lot easier to read/edit that way.

llvm/utils/TableGen/DecoderEmitter.cpp Outdated
@@ -474,8 +473,8 @@ class FilterChooser {
//
// Returns false if there exists any uninitialized bit value in the range.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update comment. It no longer matches the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

llvm/utils/TableGen/DecoderEmitter.cpp
@@ -1038,27 +1036,28 @@ void DecoderEmitter::emitDecoderFunction(formatted_raw_ostream &OS,
}
OS.indent(Indentation) << "}\n";
Indentation -= 2;
OS.indent(Indentation) << "}\n\n";
OS.indent(Indentation) << "}\n";
}

// Populates the field of the insn given the start position and the number of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

llvm/utils/TableGen/DecoderEmitter.cpp Outdated
@@ -1760,14 +1759,14 @@ bool FilterChooser::filterProcessor(bool AllowMixed, bool Greedy) {
bool AllUseless = true;
unsigned BestScore = 0;

for (unsigned i = 0, e = Filters.size(); i != e; ++i) {
unsigned Usefulness = Filters[i].usefulness();
for (const auto &[I, TFilter] : enumerate(Filters)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use "Idx" instead of "I".
What's the "T" in "TFilter" mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also TFilter renamed to Filter (agreed, looks better than the previous which meant "temp" filter).

llvm/utils/TableGen/DecoderEmitter.cpp Outdated
InOutOperands.push_back(std::pair(Out->getArg(i), Out->getArgNameStr(i)));
for (unsigned i = 0; i < In->getNumArgs(); ++i)
InOutOperands.push_back(std::pair(In->getArg(i), In->getArgNameStr(i)));
for (const auto &ArgPair :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is clearer than the two loops we had before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I've restored the two previous loops-- although I did at least use enumerate which is more concise (also eliminating the non-conforming variable name).

@jyknight
Copy link
Member

One thing I've wanted to do for a long time, but never got around to: I think we should move the long blobs of helper functions out of this file, and into a separate header which gets included from the output.

I don't think there's any real value in having them be emitted textually into the output code, vs included, and it'll be a lot easier to read/edit that way.

Sorry, I meant to add: I didn't mean that as a requirement for this PR, just a suggestion for potential future work.

@nvjle
Copy link
Contributor Author

nvjle commented Mar 12, 2024

One thing I've wanted to do for a long time, but never got around to: I think we should move the long blobs of helper functions out of this file, and into a separate header which gets included from the output.
I don't think there's any real value in having them be emitted textually into the output code, vs included, and it'll be a lot easier to read/edit that way.

Sorry, I meant to add: I didn't mean that as a requirement for this PR, just a suggestion for potential future work.

Thank you for the review, I've attended to the items mentioned.
Also, I do like your idea for future work and may take it up myself at some point.

@nvjle nvjle merged commit e9492cc into llvm:main Mar 12, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyknight jyknight jyknight approved these changes

@wangpc-pp wangpc-pp wangpc-pp approved these changes

@topperc topperc Awaiting requested review from topperc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nvjle @jyknight @wangpc-pp