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

[GlobalISel][ARM] Selection of G_CONSTANT using constant pool #96225

Closed
wants to merge 1 commit into from

Conversation

spavloff
Copy link
Collaborator

Some constants cannot be represented as immediate operands. They can be loaded from constant pool. This change implement selection of such constants in the same way as the DAG selector uses.

@llvmbot
Copy link
Member

llvmbot commented Jun 21, 2024

@llvm/pr-subscribers-backend-arm

@llvm/pr-subscribers-llvm-globalisel

Author: Serge Pavlov (spavloff)

Changes

Some constants cannot be represented as immediate operands. They can be loaded from constant pool. This change implement selection of such constants in the same way as the DAG selector uses.


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

2 Files Affected:

  • (modified) llvm/lib/Target/ARM/ARMInstructionSelector.cpp (+58)
  • (modified) llvm/test/CodeGen/ARM/GlobalISel/select-const.mir (+19)
diff --git a/llvm/lib/Target/ARM/ARMInstructionSelector.cpp b/llvm/lib/Target/ARM/ARMInstructionSelector.cpp
index 3b3c380e1e1b3..ebbf569e499e2 100644
--- a/llvm/lib/Target/ARM/ARMInstructionSelector.cpp
+++ b/llvm/lib/Target/ARM/ARMInstructionSelector.cpp
@@ -61,6 +61,10 @@ class ARMInstructionSelector : public InstructionSelector {
   bool selectSelect(MachineInstrBuilder &MIB, MachineRegisterInfo &MRI) const;
   bool selectShift(unsigned ShiftOpc, MachineInstrBuilder &MIB) const;
 
+  bool selectConstantUsingPool(MachineInstrBuilder &MIB) const;
+  void emitLoadFromConstantPool(const Register DefReg, const Constant *CPVal,
+                                MachineInstrBuilder &MIB) const;
+
   // Check if the types match and both operands have the expected size and
   // register bank.
   bool validOpRegPair(MachineRegisterInfo &MRI, unsigned LHS, unsigned RHS,
@@ -812,6 +816,57 @@ bool ARMInstructionSelector::selectShift(unsigned ShiftOpc,
   return constrainSelectedInstRegOperands(*MIB, TII, TRI, RBI);
 }
 
+bool ARMInstructionSelector::selectConstantUsingPool(
+    MachineInstrBuilder &MIB) const {
+  MachineInstr &MI = *MIB;
+  assert(MI.getOpcode() == TargetOpcode::G_CONSTANT && "Expected G_CONSTANT");
+
+  const Register DefReg = MI.getOperand(0).getReg();
+  MachineOperand &ConstOperand = MI.getOperand(1);
+  if (!ConstOperand.isCImm())
+    return false;
+  const Constant *ConstVal = ConstOperand.getCImm();
+  uint64_t ImmVal = ConstOperand.getCImm()->getZExtValue();
+
+  if (ConstantMaterializationCost(ImmVal, Subtarget) > 2 &&
+      !Subtarget->genExecuteOnly()) {
+    emitLoadFromConstantPool(DefReg, ConstVal, MIB);
+    MI.eraseFromParent();
+    return true;
+  }
+
+  return false;
+}
+
+void ARMInstructionSelector::emitLoadFromConstantPool(
+    const Register DefReg, const Constant *ConstVal,
+    MachineInstrBuilder &MIB) const {
+  MachineBasicBlock &MBB = *MIB->getParent();
+  MachineFunction &MF = *MBB.getParent();
+  const DataLayout &DL = MF.getDataLayout();
+
+  auto InsertBefore = std::next(MIB->getIterator());
+  auto &DbgLoc = MIB->getDebugLoc();
+
+  Type *ConstTy = ConstVal->getType();
+  unsigned Size = DL.getTypeStoreSize(ConstTy);
+  Align Alignment = DL.getPrefTypeAlign(ConstTy);
+
+  MachineConstantPool *Pool = MF.getConstantPool();
+  unsigned ConstIndex = Pool->getConstantPoolIndex(ConstVal, Alignment);
+
+  auto Load = BuildMI(MBB, InsertBefore, DbgLoc, TII.get(ARM::LDRcp))
+                  .addDef(DefReg)
+                  .addConstantPoolIndex(ConstIndex)
+                  .addImm(0)
+                  .add(predOps(ARMCC::AL));
+  MachinePointerInfo PtrInfo = MachinePointerInfo::getConstantPool(MF);
+  Load->addMemOperand(MF, MF.getMachineMemOperand(PtrInfo,
+                                                  MachineMemOperand::MOLoad,
+                                                  Size, Alignment));
+  constrainSelectedInstRegOperands(*Load, TII, TRI, RBI);
+}
+
 void ARMInstructionSelector::renderVFPF32Imm(
   MachineInstrBuilder &NewInstBuilder, const MachineInstr &OldInst,
   int OpIdx) const {
@@ -970,6 +1025,9 @@ bool ARMInstructionSelector::select(MachineInstr &I) {
     return selectCopy(I, TII, MRI, TRI, RBI);
   }
   case G_CONSTANT: {
+    if (selectConstantUsingPool(MIB))
+      return true;
+
     if (!MRI.getType(I.getOperand(0).getReg()).isPointer()) {
       // Non-pointer constants should be handled by TableGen.
       LLVM_DEBUG(dbgs() << "Unsupported constant type\n");
diff --git a/llvm/test/CodeGen/ARM/GlobalISel/select-const.mir b/llvm/test/CodeGen/ARM/GlobalISel/select-const.mir
index 5a1ea32a91c9e..5fd83f8937f96 100644
--- a/llvm/test/CodeGen/ARM/GlobalISel/select-const.mir
+++ b/llvm/test/CodeGen/ARM/GlobalISel/select-const.mir
@@ -23,3 +23,22 @@ body:             |
     MOVPCLR 14 /* CC::al */, $noreg, implicit $r0
 
 ...
+---
+name:            get_from_constpool
+legalized:       true
+regBankSelected: true
+selected:        false
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gprb, preferred-register: '' }
+body:             |
+  bb.1.entry:
+    ; CHECK-LABEL: name: get_from_constpool
+    ; CHECK: [[LDRcp:%[0-9]+]]:gpr = LDRcp %const.0, 0, 14 /* CC::al */, $noreg :: (load (s32) from constant-pool)
+    ; CHECK-NEXT: $r0 = COPY [[LDRcp]]
+    ; CHECK-NEXT: BX_RET 14 /* CC::al */, $noreg, implicit $r0
+    %0:gprb(s32) = G_CONSTANT i32 287454020
+    $r0 = COPY %0(s32)
+    BX_RET 14 /* CC::al */, $noreg, implicit $r0
+
+...

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.

This shouldn't really be a matter for selection, but legalization. The default lower operation for constant/fconstant is already to emit load from constant pool

@spavloff
Copy link
Collaborator Author

This implementation used the same approach as in AArch64, where loading from constant pool replaces G_CONSTANT in selector:

I could not find a use of constant pool in ARM global selector. In the DAG selector the replacement is made by the specific selecton code:

)

The difficulty of using constant pool loading at legalization stage is the specific nature of the legality predicate. In this case it should be based on the value of operand, but LegalizerInfo::getAction(const LegalityQuery &Query) does not provide such info. A possible solution is extending LegalityQuery with the information about constant arguments. Another way is to modify LegalizerHelper::legalizeInstrStep so that it allow selection of other legalization actions if some legalization step is failed. Both ways however require substantial changes and it is not clear if they agree with GlobalISel design. If you have ideas how to the legalization could be implemented, could you please share?

@arsenm
Copy link
Contributor

arsenm commented Jun 24, 2024

You can custom lower, check the value and then use the LegalizerHelper lower implementation if you need to expand it. Alternatively, we can make the default lower implementation for constant check the isImmLegal or whatever it's called TargetLowering hook

@spavloff
Copy link
Collaborator Author

The alternative implementation is provided in #98308, it uses legalization instead of instruction selection.

Some constants cannot be represented as immediate operands. They can
be loaded from constant pool. This change implement selection of such
constants in the same way as the DAG selector uses.
@spavloff
Copy link
Collaborator Author

Abandon in favour of #98308.

@spavloff spavloff closed this Oct 14, 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.

3 participants