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.

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

[EVM] Refactor EVMControlFlowGraph/EVMControlFlowGraphBuilder #750

Conversation

PavelKopyl
Copy link
Contributor

  • Removed EVMControlFlowGraph/EVMControlFlowGraphBuilder classes
  • Functionality that analyzes machine CFG and provides query methods was moved to EVMMachineCFGInfo class
  • Information about MBB terminators is represented in EVMMBBTerminatorsInfo class
  • StackSlot, Stack and Operation definitions were moved to EVMStackModel
  • Replaced almost all the std::map/std::set with llvm counterparts in EVMStackLayoutGenerator

Code Review Checklist

Purpose

Ticket Number

Requirements

  • Have the requirements been met?
  • Have stakeholder(s) approved the change?

Implementation

  • Does this code change accomplish what it is supposed to do?
  • Can this solution be simplified?
  • Does this change add unwanted compile-time or run-time dependencies?
  • Could an additional framework, API, library, or service improve the solution?
  • Could we reuse part of LLVM instead of implementing the patch or a part of it?
  • Is the code at the right abstraction level?
  • Is the code modular enough?
  • Can a better solution be found in terms of maintainability, readability, performance, or security?
  • Does similar functionality already exist in the codebase? If yes, why isn’t it reused?
  • Are there any best practices, design patterns or language-specific patterns that could substantially improve this code?

Logic Errors and Bugs

  • Can you think of any use case in which the
    code does not behave as intended?
  • Can you think of any inputs or external events
    that could break the code?

Error Handling and Logging

  • Is error handling done the correct way?
  • Should any logging or debugging information
    be added or removed?
  • Are error messages user-friendly?
  • Are there enough log events and are they
    written in a way that allows for easy
    debugging?

Maintainability

  • Is the code easy to read?
  • Is the code not repeated (DRY Principle)?
  • Is the code method/class not too long?

Dependencies

  • Were updates to documentation, configuration, or readme files made as required by this change?
  • Are there any potential impacts on other parts of the system or backward compatibility?

Security

  • Does the code introduce any security vulnerabilities?

Performance

  • Do you think this code change decreases
    system performance?
  • Do you see any potential to improve the
    performance of the code significantly?

Testing and Testability

  • Is the code testable?
  • Have automated tests been added, or have related ones been updated to cover the change?
    • For changes to mutable state
  • Do tests reasonably cover the code change (unit/integration/system tests)?
    • Line Coverage
    • Region Coverage
    • Branch Coverage
  • Are there some test cases, input or edge cases
    that should be tested in addition?

Readability

  • Is the code easy to understand?
  • Which parts were confusing to you and why?
  • Can the readability of the code be improved by
    smaller methods?
  • Can the readability of the code be improved by
    different function, method or variable names?
  • Is the code located in the right
    file/folder/package?
  • Do you think certain methods should be
    restructured to have a more intuitive control
    flow?
  • Is the data flow understandable?
  • Are there redundant or outdated comments?
  • Could some comments convey the message
    better?
  • Would more comments make the code more
    understandable?
  • Could some comments be removed by making the code itself more readable?
  • Is there any commented-out code?
  • Have you run a spelling and grammar checker?

Documentation

  • Is there sufficient documentation?
  • Is the ReadMe.md file up to date?

Best Practices

  • Follow Single Responsibility principle?
  • Are different errors handled correctly?
  • Are errors and warnings logged?
  • Magic values avoided?
  • No unnecessary comments?
  • Minimal nesting used?

Experts' Opinion

  • Do you think a specific expert, like a security
    expert or a usability expert, should look over
    the code before it can be accepted?
  • Will this code change impact different teams, and should they review the change as well?

Copy link
Contributor

@vladimirradosavljevic vladimirradosavljevic left a comment

Choose a reason for hiding this comment

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

Overall, the code looks good. I just have a few comments.

llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.h Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.h Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.h Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackifyCodeEmitter.cpp Outdated Show resolved Hide resolved
@PavelKopyl
Copy link
Contributor Author

Hey @vladimirradosavljevic, thank you for the review. I agree with all the notes. Please find the fixes in the last commit.

@PavelKopyl PavelKopyl changed the title [DO NOT MERGE][EVM] Refactor EVMControlFlowGraph/EVMControlFlowGraphBuilder [EVM] Refactor EVMControlFlowGraph/EVMControlFlowGraphBuilder Jan 10, 2025
Copy link
Collaborator

@akiramenai akiramenai left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
}

static bool isTerminate(const MachineInstr *MI) {
unsigned Opc = MI->getOpcode();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: unnecessary temporary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

Copy link
Contributor

@vladimirradosavljevic vladimirradosavljevic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Just small nitpicking.

llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMMachineCFGInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackLayoutGenerator.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackLayoutGenerator.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackLayoutGenerator.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackLayoutGenerator.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/EVM/EVMStackLayoutGenerator.cpp Outdated Show resolved Hide resolved
@akiramenai
Copy link
Collaborator

Please squash and merge

- Removed EVMControlFlowGraph/EVMControlFlowGraphBuilder classes
- Functionality that analyzes machine CFG and provides query methods
  was moved to EVMMachineCFGInfo class
- Information about MBB terminators is represented in EVMMBBTerminatorsInfo
  class
- StackSlot, Stack and Operation definitions  were moved to EVMStackModel
- Replaced almost all the std::map/std::set with llvm counterparts in
  EVMStackLayoutGenerator
@PavelKopyl PavelKopyl force-pushed the kpv-cpr-1831-evm-replace-controlflowbuilder-with-map-and-query-functions branch from d5d4939 to 0b13fca Compare January 13, 2025 21:23
@PavelKopyl PavelKopyl merged commit f5dde02 into ef-stackification Jan 13, 2025
7 checks passed
@PavelKopyl PavelKopyl deleted the kpv-cpr-1831-evm-replace-controlflowbuilder-with-map-and-query-functions branch January 13, 2025 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants