Skip to content

[AArch64][GISel] Use Register instead of MCRegister for markPhysRegUsed in CallLowering. #122853

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

Merged
merged 3 commits into from
Jan 16, 2025

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Jan 14, 2025

For "returned" attribute arguments, the physical register is really a
virtual register which shouldn't be stored in an MCRegister. This
moves the conversion from Register to MCRegister into the derived
classes of IncomingArgHandler. The derived class ReturnedArgCallReturnHandler
does not use the register so no MCRegister is created in that case.
The function and argument have been renamed to remove "Phys".

…ed in CallLowering.

For "returned" attribute arguments, the physical register is a
virtual register which shouldn't be stored in MCRegister. This
moves the conversion from Register to MCRegister into the derived
classes of IncomingArgHandler. The derived class ReturnedArgCallReturnHandler
does not use the register so no MCRegister is created in that case.
@llvmbot
Copy link
Member

llvmbot commented Jan 14, 2025

@llvm/pr-subscribers-backend-aarch64

Author: Craig Topper (topperc)

Changes

For "returned" attribute arguments, the physical register is really a
virtual register which shouldn't be stored in an MCRegister. This
moves the conversion from Register to MCRegister into the derived
classes of IncomingArgHandler. The derived class ReturnedArgCallReturnHandler
does not use the register so no MCRegister is created in that case.

I can remove Phys from function name for clarity if we want.


Full diff: https://github.com/llvm/llvm-project/pull/122853.diff

1 Files Affected:

  • (modified) llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp (+4-4)
diff --git a/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp b/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
index 15f1c99e87246b..3fceb922f35520 100644
--- a/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
+++ b/llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
@@ -207,14 +207,14 @@ struct IncomingArgHandler : public CallLowering::IncomingValueHandler {
   /// How the physical register gets marked varies between formal
   /// parameters (it's a basic-block live-in), and a call instruction
   /// (it's an implicit-def of the BL).
-  virtual void markPhysRegUsed(MCRegister PhysReg) = 0;
+  virtual void markPhysRegUsed(Register PhysReg) = 0;
 };
 
 struct FormalArgHandler : public IncomingArgHandler {
   FormalArgHandler(MachineIRBuilder &MIRBuilder, MachineRegisterInfo &MRI)
       : IncomingArgHandler(MIRBuilder, MRI) {}
 
-  void markPhysRegUsed(MCRegister PhysReg) override {
+  void markPhysRegUsed(Register PhysReg) override {
     MIRBuilder.getMRI()->addLiveIn(PhysReg);
     MIRBuilder.getMBB().addLiveIn(PhysReg);
   }
@@ -225,7 +225,7 @@ struct CallReturnHandler : public IncomingArgHandler {
                     MachineInstrBuilder MIB)
       : IncomingArgHandler(MIRBuilder, MRI), MIB(MIB) {}
 
-  void markPhysRegUsed(MCRegister PhysReg) override {
+  void markPhysRegUsed(Register PhysReg) override {
     MIB.addDef(PhysReg, RegState::Implicit);
   }
 
@@ -239,7 +239,7 @@ struct ReturnedArgCallReturnHandler : public CallReturnHandler {
                                MachineInstrBuilder MIB)
       : CallReturnHandler(MIRBuilder, MRI, MIB) {}
 
-  void markPhysRegUsed(MCRegister PhysReg) override {}
+  void markPhysRegUsed(Register PhysReg) override {}
 };
 
 struct OutgoingArgHandler : public CallLowering::OutgoingValueHandler {

};

struct FormalArgHandler : public IncomingArgHandler {
FormalArgHandler(MachineIRBuilder &MIRBuilder, MachineRegisterInfo &MRI)
: IncomingArgHandler(MIRBuilder, MRI) {}

void markPhysRegUsed(MCRegister PhysReg) override {
void markPhysRegUsed(Register PhysReg) override {
MIRBuilder.getMRI()->addLiveIn(PhysReg);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the confusion here is deeper, and I'm not sure a virtual register should reach here. addLiveIn for a virtual register also doesn't make sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not a virtual register when FormalArgHandler::markPhysRegUsed is called. It's only virtual when ReturnedArgCallReturnHandler::markPhysRegUsed is called. This is a virtual function.

Copy link
Collaborator Author

@topperc topperc Jan 14, 2025

Choose a reason for hiding this comment

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

I've changed to use Register::asMCReg() instead of implicit conversion for the addLiveIn calls. That will check that it is safe to convert from Register to MCRegister.

@topperc topperc requested review from arsenm and davemgreen January 14, 2025 18:03
@topperc topperc merged commit 383eb0b into llvm:main Jan 16, 2025
8 checks passed
@topperc topperc deleted the pr/aarch64-markPhysRegUsed branch January 16, 2025 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants