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

[CodeGen] Refactor MachineInstr findRegisterDefOperandIdx/findRegisterDefOperand etc. to stop missing TRI args #82659

Closed
RKSimon opened this issue Feb 22, 2024 · 8 comments · Fixed by #85968
Assignees
Labels
good first issue https://github.com/llvm/llvm-project/contribute llvm:codegen

Comments

@RKSimon
Copy link
Collaborator

RKSimon commented Feb 22, 2024

As noticed by @AtariDreams on #82411, by having the TargetRegisterInfo argument at the end of a long line of optional args, we are often missing it in our function calls and not correctly testing for super reg kill state etc.

  int findRegisterUseOperandIdx(Register Reg, bool isKill = false, const TargetRegisterInfo *TRI = nullptr) const;
  MachineOperand *findRegisterUseOperand(Register Reg, bool isKill = false, const TargetRegisterInfo *TRI = nullptr);
  ... etc ...

ideally we need to refactor the calls so the TRI is no longer optional, so when we don't care about super-registers we explicitly have to set it nullptr.

  int findRegisterUseOperandIdx(Register Reg, const TargetRegisterInfo *TRI, bool isKill = false) const;
  MachineOperand *findRegisterUseOperand(Register Reg, const TargetRegisterInfo *TRI, bool isKill = false);
  ... etc ...
@RKSimon RKSimon added good first issue https://github.com/llvm/llvm-project/contribute llvm:codegen labels Feb 22, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 22, 2024

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. In the comments of the issue, request for it to be assigned to you.
  2. Fix the issue locally.
  3. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  4. Create a Git commit.
  5. Run git clang-format HEAD~1 to format your changes.
  6. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 22, 2024

@llvm/issue-subscribers-good-first-issue

Author: Simon Pilgrim (RKSimon)

As noticed by @AtariDreams on #82411, by having the TargetRegisterInfo argument at the end of a long line of optional args, we are often missing it in our function calls and not correctly testing for super reg kill state etc.
  int findRegisterUseOperandIdx(Register Reg, bool isKill = false, const TargetRegisterInfo *TRI = nullptr) const;
  MachineOperand *findRegisterUseOperand(Register Reg, bool isKill = false, const TargetRegisterInfo *TRI = nullptr);
  ... etc ...

ideally we need to refactor the calls so the TRI is no longer optional, so when we don't care about super-registers we explicitly have to set it nullptr.

  int findRegisterUseOperandIdx(Register Reg, const TargetRegisterInfo *TRI, bool isKill = false) const;
  MachineOperand *findRegisterUseOperand(Register Reg, const TargetRegisterInfo *TRI, bool isKill = false);
  ... etc ...

@simonzgx
Copy link
Contributor

Hi! I am new to the LLVM project, and I would like to take this issue. Could anyone please assign this issue to me?

@RKSimon
Copy link
Collaborator Author

RKSimon commented Feb 25, 2024

@simonzgx Please keep an eye on #82411 as @AtariDreams is adding explicit TRI to several locations to address some implicit bugs - its likely that patch will land first so you may have to rebase a large number of conflicts

@simonzgx
Copy link
Contributor

@RKSimon Got it.

@RKSimon
Copy link
Collaborator Author

RKSimon commented Mar 19, 2024

@simonzgx #82411 has stalled due to test coverage issues, so if you are able to get your patch updated we might be able to get it in before it.

@simonzgx
Copy link
Contributor

@RKSimon Okay, will do it pretty soon.

@simonzgx
Copy link
Contributor

Hi @RKSimon ! As it mentioned, I submitted a new PR(#85968) and ready to be reviewed, please have a look.

RKSimon pushed a commit that referenced this issue Apr 24, 2024
Fixes #82659

There are some functions, such as `findRegisterDefOperandIdx` and  `findRegisterDefOperand`, that have too many default parameters. As a result, we have encountered some issues due to the lack of TRI  parameters, as shown in issue #82411.

Following @RKSimon 's suggestion, this patch refactors 9 functions, including `{reads, kills, defines, modifies}Register`,  `registerDefIsDead`, and `findRegister{UseOperandIdx, UseOperand, DefOperandIdx, DefOperand}`, adjusting the order of the TRI parameter and making it required. In addition, all the places that call these functions have also been updated correctly to ensure no additional impact.

After this, the caller of these functions should explicitly know whether to pass the `TargetRegisterInfo` or just a `nullptr`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue https://github.com/llvm/llvm-project/contribute llvm:codegen
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants