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

[RISCV][GlobalISel] Select G_GLOBAL_VALUE for medlow code model #68380

Closed
wants to merge 4 commits into from

Conversation

nitinjohnraj
Copy link
Contributor

@nitinjohnraj nitinjohnraj commented Oct 6, 2023

We create generic instructions to serve as globalisel equivalents to selection DAG nodes, in order to leverage the SelectionDAG patterns. We lower G_GLOBAL_VALUE to these generic instructions.

@nitinjohnraj
Copy link
Contributor Author

Currently, this patch does not compile. I'm trying to figure out which file to import in order to use the new generic instruction opcodes in the instruction selector. Also, I'm only trying to handle the medlow code model for now. I intend to add code for medany as well.

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff ce8ed0008713781e58777d251f654a9a547f43b7 b1f7484e6ca571121ffeffd28316b48e6befb59c -- llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
View the diff from clang-format here.
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
index a63e9d111d9d..650b186a5626 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
@@ -359,8 +359,8 @@ bool RISCVInstructionSelector::selectConstant(MachineInstr &MI,
   return true;
 }
 
-bool RISCVInstructionSelector::selectGlobalValue(MachineInstr &MI, MachineIRBuilder &MIB,
-                                                 MachineRegisterInfo &MRI) const {
+bool RISCVInstructionSelector::selectGlobalValue(
+    MachineInstr &MI, MachineIRBuilder &MIB, MachineRegisterInfo &MRI) const {
   assert(MI.getOpcode() == TargetOpcode::G_GLOBAL_VALUE &&
          "Wrong selector used!");
 
@@ -369,14 +369,15 @@ bool RISCVInstructionSelector::selectGlobalValue(MachineInstr &MI, MachineIRBuil
     Register DstReg = MI.getOperand(0).getReg();
     const GlobalValue *GV = MI.getOperand(1).getGlobal();
     Register TmpReg = MRI.createVirtualRegister(&RISCV::GPRRegClass);
-    MachineInstr *Result =
-        MIB.buildInstr(RISCV::LUI).addDef(TmpReg).addGlobalAddress(GV, 0, RISCVII::MO_HI);
+    MachineInstr *Result = MIB.buildInstr(RISCV::LUI)
+                               .addDef(TmpReg)
+                               .addGlobalAddress(GV, 0, RISCVII::MO_HI);
 
     if (!constrainSelectedInstRegOperands(*Result, TII, TRI, RBI))
       return false;
 
     Result = MIB.buildInstr(RISCV::ADDI)
-      .addDef(DstReg)
+                 .addDef(DstReg)
                  .addReg(TmpReg)
                  .addGlobalAddress(GV, 0, RISCVII::MO_LO);
 

let Namespace = "RISCV";
}

def G_ADD_LO : RISCVGenericInstruction {
Copy link
Contributor

Choose a reason for hiding this comment

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

G_RISCV_?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I abandoned using these generic instructions. If you look at the next comment, you'll see they were deleted; I didn't squash the comment so that I could preserve that version in case we decided to revert to that.

In the future, would it be better if I squashed such changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we decide to go with the generic instruction approach, I will rename them accordingly, though, it does make things much clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I abandoned using these generic instructions

These look to be part of the overall diff still. Do you plan on removing the changes in this file if you are not going to use them?

@nitinjohnraj nitinjohnraj changed the title [RISCV][GlobalISel] Select G_GLOBAL_VALUE [RISCV][GlobalISel] Select G_GLOBAL_VALUE for medlow code model Oct 6, 2023
@@ -353,6 +359,38 @@ bool RISCVInstructionSelector::selectConstant(MachineInstr &MI,
return true;
}

bool RISCVInstructionSelector::selectGlobalValue(MachineInstr &MI, MachineIRBuilder &MIB,
Copy link
Contributor

Choose a reason for hiding this comment

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

This function needs to be clang-format'ed

michaelmaitland added a commit to michaelmaitland/llvm-project that referenced this pull request Oct 24, 2023
G_GLOBAL_VALUE should be lowered into an absolute address if `-codemodel=small`
is used or into a PC-relative if `-codemodel=medium` is used.

PR llvm#68380 tried to create special instructions to do this, but I don't
see why we need to do that.
michaelmaitland added a commit that referenced this pull request Oct 30, 2023
G_GLOBAL_VALUE should be lowered into an absolute address if
`-codemodel=small` is used or into a PC-relative if `-codemodel=medium`
is used.

PR #68380 tried to create special instructions to do this, but I don't
see why we need to do that.
Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

Requires resolve to main

@topperc topperc closed this Feb 6, 2024
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.

None yet

5 participants