Skip to content

[X86][APX] Fix issues of suppressing APX for relocation #139285

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.

Sign up for GitHub

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 7 commits into from
May 12, 2025
Merged

[X86][APX] Fix issues of suppressing APX for relocation #139285

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1f638bd
[X86][APX] Fix issues of suppressing APX for relocation
fzou1 May 8, 2025
a078f6e
Add fix for another issue
fzou1 May 10, 2025
b22127b
Refine the code and update test
fzou1 May 11, 2025
fe18e76
Add comments
fzou1 May 11, 2025
682707a
Rename test
fzou1 May 11, 2025
b735238
Suppress EGPR in all of register uses.
fzou1 May 11, 2025
088a135
Limit updating reg uses for PHI instruction and update tests.
fzou1 May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions llvm/lib/Target/X86/X86InstrInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8122,6 +8122,14 @@ MachineInstr *X86InstrInfo::foldMemoryOperandImpl(
shouldPreventUndefRegUpdateMemFold(MF, MI)))
return nullptr;

// Do not fold a NDD instruction and a memory instruction with relocation to
// avoid emit APX relocation when the flag is disabled for backward
// compatibility.
uint64_t TSFlags = MI.getDesc().TSFlags;
if (!X86EnableAPXForRelocation && isMemInstrWithGOTPCREL(LoadMI) &&
X86II::hasNewDataDest(TSFlags))
return nullptr;

// Determine the alignment of the load.
Align Alignment;
unsigned LoadOpc = LoadMI.getOpcode();
Expand Down
34 changes: 34 additions & 0 deletions llvm/lib/Target/X86/X86InstrInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,40 @@ inline static bool isAddMemInstrWithRelocation(const MachineInstr &MI) {
return false;
}

inline static bool isMemInstrWithGOTPCREL(const MachineInstr &MI) {
unsigned Op = MI.getOpcode();
switch (Op) {
case X86::TEST32mr:
case X86::TEST64mr:
case X86::CMP32rm:
case X86::CMP64rm:
case X86::MOV32rm:
case X86::MOV64rm:
case X86::ADC32rm:
case X86::ADD32rm:
case X86::AND32rm:
case X86::OR32rm:
case X86::SBB32rm:
case X86::SUB32rm:
case X86::XOR32rm:
case X86::ADC64rm:
case X86::ADD64rm:
case X86::AND64rm:
case X86::OR64rm:
case X86::SBB64rm:
case X86::SUB64rm:
case X86::XOR64rm: {
int MemOpNo = X86II::getMemoryOperandNo(MI.getDesc().TSFlags) +
X86II::getOperandBias(MI.getDesc());
const MachineOperand &MO = MI.getOperand(X86::AddrDisp + MemOpNo);
if (MO.getTargetFlags() == X86II::MO_GOTPCREL)
return true;
break;
}
}
return false;
}

class X86InstrInfo final : public X86GenInstrInfo {
X86Subtarget &Subtarget;
const X86RegisterInfo RI;
Expand Down
22 changes: 22 additions & 0 deletions llvm/lib/Target/X86/X86RegisterInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ static cl::opt<bool>
cl::desc("Disable two address hints for register "
"allocation"));

extern cl::opt<bool> X86EnableAPXForRelocation;

X86RegisterInfo::X86RegisterInfo(const Triple &TT)
: X86GenRegisterInfo((TT.isArch64Bit() ? X86::RIP : X86::EIP),
X86_MC::getDwarfRegFlavour(TT, false),
Expand Down Expand Up @@ -121,6 +123,11 @@ X86RegisterInfo::getLargestLegalSuperClass(const TargetRegisterClass *RC,
if (RC == &X86::GR8_NOREXRegClass)
return RC;

// Keep using non-rex2 register class when APX feature (EGPR/NDD/NF) is not
// enabled for relocation.
if (!X86EnableAPXForRelocation && isNonRex2RegClass(RC))
return RC;

const X86Subtarget &Subtarget = MF.getSubtarget<X86Subtarget>();

const TargetRegisterClass *Super = RC;
Expand Down Expand Up @@ -1258,3 +1265,18 @@ const TargetRegisterClass *X86RegisterInfo::constrainRegClassToNonRex2(
return &X86::GR64_NOREX2_NOSPRegClass;
}
}

bool X86RegisterInfo::isNonRex2RegClass(const TargetRegisterClass *RC) const {
switch (RC->getID()) {
default:
return false;
case X86::GR8_NOREX2RegClassID:
case X86::GR16_NOREX2RegClassID:
case X86::GR32_NOREX2RegClassID:
case X86::GR64_NOREX2RegClassID:
case X86::GR32_NOREX2_NOSPRegClassID:
case X86::GR64_NOREX2_NOSPRegClassID:
case X86::GR64_with_sub_16bit_in_GR16_NOREX2RegClassID:
return true;
}
}
2 changes: 2 additions & 0 deletions llvm/lib/Target/X86/X86RegisterInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ class X86RegisterInfo final : public X86GenRegisterInfo {

const TargetRegisterClass *
constrainRegClassToNonRex2(const TargetRegisterClass *RC) const;

bool isNonRex2RegClass(const TargetRegisterClass *RC) const;
};

} // End llvm namespace
Expand Down
33 changes: 27 additions & 6 deletions llvm/lib/Target/X86/X86SuppressAPXForReloc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,8 @@ FunctionPass *llvm::createX86SuppressAPXForRelocationPass() {
return new X86SuppressAPXForRelocationPass();
}

static void suppressEGPRRegClass(MachineFunction &MF, MachineInstr &MI,
static void suppressEGPRRegClass(MachineRegisterInfo *MRI, MachineInstr &MI,
const X86Subtarget &ST, unsigned int OpNum) {
MachineRegisterInfo *MRI = &MF.getRegInfo();
Register Reg = MI.getOperand(OpNum).getReg();
if (!Reg.isVirtual()) {
assert(!X86II::isApxExtendedReg(Reg) && "APX EGPR is used unexpectedly.");
Expand All @@ -79,11 +78,32 @@ static void suppressEGPRRegClass(MachineFunction &MF, MachineInstr &MI,
MRI->setRegClass(Reg, NewRC);
}

// Suppress EGPR in operand 0 of uses to avoid APX relocation types emitted. The
// register in operand 0 of instruction with relocation may be replaced with
// operand 0 of uses which may be EGPR. That may lead to emit APX relocation
// types which breaks the backward compatibility with builtin linkers on
// existing OS. For example, the register in operand 0 of instruction with
// relocation is used in PHI instruction, and it may be replaced with operand 0
// of PHI instruction after PHI elimination and Machine Copy Propagation pass.
static void suppressEGPRRegClassInRegAndUses(MachineRegisterInfo *MRI,
MachineInstr &MI,
const X86Subtarget &ST,
unsigned int OpNum) {
suppressEGPRRegClass(MRI, MI, ST, OpNum);
Register Reg = MI.getOperand(OpNum).getReg();
for (MachineInstr &Use : MRI->use_instructions(Reg)) {
const unsigned UseOpNum = 0;
if (Use.getOperand(UseOpNum).isReg())
Copy link
Contributor

@phoebewang phoebewang May 12, 2025

Choose a reason for hiding this comment

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

Why operand 0 instead of the operand uses the same register?

Copy link
Contributor Author

@fzou1 fzou1 May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The operand which uses the same register is automatically updated when it's set to non-rex2 register class. The IR after Suppress APX for relocation pass:

bb.0.alloca_15:
  %29:gr64_norex2 = MOV64rm $rip, 1, $noreg, target-flags(x86-gotpcrel) @gvar, $noreg :: (load (s64) from got)
…
bb.1.loop.253:
…
  %6:gr64 = PHI %29:gr64_norex2, %bb.0, %9:gr64, %bb.3
…

But the register class of operand 0 (%6) in PHI instruction using %29 is gr64. It may replace the operand 0 in MOV64rm instruction with gotpcrel relocation. See the output in the passes:

After Eliminate PHI nodes for register allocation

bb.0.alloca_15:
  %29:gr64_norex2 = MOV64rm $rip, 1, $noreg, target-flags(x86-gotpcrel) @gvar, $noreg :: (load (s64) from got)
  %42:gr64 = COPY %29:gr64_norex2

bb.1.loop.253:
  %6:gr64 = COPY killed %42:gr64

During Machine Copy Propagation Pass

MCP: Replacing $r15                                                             
     with $r16                                                                  
     in renamable $r15 = MOV64rm $rip, 1, $noreg, target-flags(x86-gotpcrel) @gvar, $noreg :: (load (s64) from got)
     from renamable $r16 = COPY killed renamable $r15                           
MCP: After replacement: renamable $r16 = MOV64rm $rip, 1, $noreg, target-flags(x86-gotpcrel) @gvar, $noreg :: (load (s64) from got)

After Machine Copy Propagation Pass

bb.0.alloca_15:
  renamable $r16 = MOV64rm $rip, 1, $noreg, target-flags(x86-gotpcrel) @gvar, $noreg :: (load (s64) from got)

So the operand 0 of PHI instruction should be updated to non-rex2 register class, to avoid APX relocation types emitted.

Copy link
Contributor

@phoebewang phoebewang May 12, 2025

Choose a reason for hiding this comment

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

It looks like a general problem to me. We have some instructions not promoted to APX encoding, e.g., BSR/BSF. They should have the same problem. @KanRobert is it a design defect or something wrong in some where?

Copy link
Contributor

@KanRobert KanRobert May 12, 2025

Choose a reason for hiding this comment

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

IIRC, for the existing optimizations and cg, the common register class will be used, aka. gr64 + gr64_norex2 -> gr64_norex2. If we find this problem here, probably it's brought by the APX suppressing patches.

Copy link
Contributor

@phoebewang phoebewang May 12, 2025

Choose a reason for hiding this comment

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

Thanks @fzou1 and @KanRobert , then I think it's better to limit to PHI instrcution only for now.

suppressEGPRRegClass(MRI, Use, ST, UseOpNum);
}
}

static bool handleInstructionWithEGPR(MachineFunction &MF,
const X86Subtarget &ST) {
if (!ST.hasEGPR())
return false;

MachineRegisterInfo *MRI = &MF.getRegInfo();
auto suppressEGPRInInstrWithReloc = [&](MachineInstr &MI,
ArrayRef<unsigned> OpNoArray) {
int MemOpNo = X86II::getMemoryOperandNo(MI.getDesc().TSFlags) +
Expand All @@ -94,7 +114,7 @@ static bool handleInstructionWithEGPR(MachineFunction &MF,
LLVM_DEBUG(dbgs() << "Transform instruction with relocation type:\n "
<< MI);
for (unsigned OpNo : OpNoArray)
suppressEGPRRegClass(MF, MI, ST, OpNo);
suppressEGPRRegClassInRegAndUses(MRI, MI, ST, OpNo);
LLVM_DEBUG(dbgs() << "to:\n " << MI << "\n");
}
};
Expand Down Expand Up @@ -167,7 +187,8 @@ static bool handleNDDOrNFInstructions(MachineFunction &MF,
int MemOpNo = X86II::getMemoryOperandNo(MI.getDesc().TSFlags) +
X86II::getOperandBias(MI.getDesc());
const MachineOperand &MO = MI.getOperand(X86::AddrDisp + MemOpNo);
if (MO.getTargetFlags() == X86II::MO_GOTTPOFF) {
if (MO.getTargetFlags() == X86II::MO_GOTTPOFF ||
MO.getTargetFlags() == X86II::MO_GOTPCREL) {
LLVM_DEBUG(dbgs() << "Transform instruction with relocation type:\n "
<< MI);
Register Reg = MRI->createVirtualRegister(&X86::GR64_NOREX2RegClass);
Expand All @@ -178,7 +199,7 @@ static bool handleNDDOrNFInstructions(MachineFunction &MF,
MI.getOperand(1).setReg(Reg);
const MCInstrDesc &NewDesc = TII->get(X86::ADD64rm);
MI.setDesc(NewDesc);
suppressEGPRRegClass(MF, MI, ST, 0);
suppressEGPRRegClassInRegAndUses(MRI, MI, ST, 0);
MI.tieOperands(0, 1);
LLVM_DEBUG(dbgs() << "to:\n " << *CopyMIB << "\n");
LLVM_DEBUG(dbgs() << " " << MI << "\n");
Expand All @@ -191,7 +212,7 @@ static bool handleNDDOrNFInstructions(MachineFunction &MF,
if (MO.getTargetFlags() == X86II::MO_GOTTPOFF) {
LLVM_DEBUG(dbgs() << "Transform instruction with relocation type:\n "
<< MI);
suppressEGPRRegClass(MF, MI, ST, 0);
suppressEGPRRegClassInRegAndUses(MRI, MI, ST, 0);
Register Reg = MRI->createVirtualRegister(&X86::GR64_NOREX2RegClass);
[[maybe_unused]] MachineInstrBuilder CopyMIB =
BuildMI(MBB, MI, MI.getDebugLoc(), TII->get(TargetOpcode::COPY),
Expand Down
Loading