Skip to content

[X86][GlobalISel] - Legalize And Select of G_FPTOSI/G_SITOFP in X87 mode #137377

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
e-kud merged 11 commits into from
Jun 2, 2025
Merged

[X86][GlobalISel] - Legalize And Select of G_FPTOSI/G_SITOFP in X87 mode #137377

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1415d66
[X86][GlobalISel] - Legalize And Select of G_FPTOSI/G_SITOFP in X87 mode
pawan-nirpal-031 Apr 25, 2025
2dcb057
addressing cmts
pawan-nirpal-031 May 13, 2025
1ca8ff2
introduce predicated customForCartesianProduct
pawan-nirpal-031 May 14, 2025
4d9944d
braces for legalfor
pawan-nirpal-031 May 14, 2025
c62687d
fix failing tests
pawan-nirpal-031 May 14, 2025
10bc9bf
fix tests
pawan-nirpal-031 May 21, 2025
1e910d3
Update llvm/lib/Target/X86/X86InstrGISel.td
pawan-nirpal-031 May 23, 2025
c1bb7af
use MemoryVT instead of custom c++ predicate
pawan-nirpal-031 May 23, 2025
919c934
oops
pawan-nirpal-031 May 23, 2025
9821d76
remove redundant include
pawan-nirpal-031 May 29, 2025
19b359e
Merge branch 'main' into gisel-sitofp-fptosi-x87-enablement
pawan-nirpal-031 May 30, 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: 3 additions & 5 deletions llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class X86InstructionSelector : public InstructionSelector {
unsigned getPtrLoadStoreOp(const LLT &Ty, const RegisterBank &RB,
unsigned Opc) const;

bool checkMemoryOpSize(const MachineInstr &MI, unsigned NumBytes) const;
bool checkMemoryOpSize(const MachineInstr &MI, LLT Sz) const;

bool selectLoadStoreOp(MachineInstr &I, MachineRegisterInfo &MRI,
MachineFunction &MF) const;
Expand Down Expand Up @@ -358,12 +358,10 @@ bool X86InstructionSelector::selectCopy(MachineInstr &I,
}

bool X86InstructionSelector::checkMemoryOpSize(const MachineInstr &MI,
unsigned NumBytes) const {
if (!MI.mayLoadOrStore())
return false;
LLT Sz) const {
assert(MI.hasOneMemOperand() &&
"Expected load/store to have only one mem op!");
return (*MI.memoperands_begin())->getSize() == NumBytes;
return (*MI.memoperands_begin())->getMemoryType() == Sz;
}

bool X86InstructionSelector::select(MachineInstr &I) {
Expand Down
83 changes: 34 additions & 49 deletions llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,46 +491,38 @@ X86LegalizerInfo::X86LegalizerInfo(const X86Subtarget &STI,
});

getActionDefinitionsBuilder(G_SITOFP)
.legalIf([=](const LegalityQuery &Query) {
return (HasSSE1 &&
(typePairInSet(0, 1, {{s32, s32}})(Query) ||
(Is64Bit && typePairInSet(0, 1, {{s32, s64}})(Query)))) ||
(HasSSE2 &&
(typePairInSet(0, 1, {{s64, s32}})(Query) ||
(Is64Bit && typePairInSet(0, 1, {{s64, s64}})(Query))));
})
.customIf([=](const LegalityQuery &Query) -> bool {
if (!UseX87)
return false;
if ((typeIs(0, s32)(Query) && HasSSE1) ||
(typeIs(0, s64)(Query) && HasSSE2))
return false;
return typeInSet(0, {s32, s64, s80})(Query) &&
typeInSet(1, {s16, s32, s64})(Query);
})
.legalFor(HasSSE1, {s32, s32})
Copy link
Collaborator

@RKSimon RKSimon May 13, 2025

Choose a reason for hiding this comment

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

Should this be: .legalFor(HasSSE1, {{s32, s32}}) for the dst/src pair?

.legalFor(HasSSE1 && Is64Bit, {s32, s64})
.legalFor(HasSSE2, {s64, s32})
.legalFor(HasSSE2 && Is64Bit, {s64, s64})
.customFor(UseX87, {{s32, s16},
{s32, s32},
{s32, s64},
{s64, s16},
{s64, s32},
{s64, s64},
{s80, s16},
{s80, s32},
{s80, s64}})
.clampScalar(1, (UseX87 && !HasSSE1) ? s16 : s32, sMaxScalar)
.widenScalarToNextPow2(1)
.clampScalar(0, s32, HasSSE2 ? s64 : s32)
.widenScalarToNextPow2(0);

getActionDefinitionsBuilder(G_FPTOSI)
.legalIf([=](const LegalityQuery &Query) {
return (HasSSE1 &&
(typePairInSet(0, 1, {{s32, s32}})(Query) ||
(Is64Bit && typePairInSet(0, 1, {{s64, s32}})(Query)))) ||
(HasSSE2 &&
(typePairInSet(0, 1, {{s32, s64}})(Query) ||
(Is64Bit && typePairInSet(0, 1, {{s64, s64}})(Query))));
})
.customIf([=](const LegalityQuery &Query) -> bool {
if (!UseX87)
return false;
if ((typeIs(1, s32)(Query) && HasSSE1) ||
(typeIs(1, s64)(Query) && HasSSE2))
return false;
return typeInSet(0, {s16, s32, s64})(Query) &&
typeInSet(1, {s32, s64, s80})(Query);
})
.legalFor(HasSSE1, {s32, s32})
.legalFor(HasSSE1 && Is64Bit, {s64, s32})
.legalFor(HasSSE2, {s32, s64})
.legalFor(HasSSE2 && Is64Bit, {s64, s64})
.customFor(UseX87, {{s16, s32},
{s16, s64},
{s16, s80},
{s32, s32},
{s32, s64},
{s32, s80},
{s64, s32},
{s64, s64},
{s64, s80}})
.clampScalar(0, (UseX87 && !HasSSE1) ? s16 : s32, sMaxScalar)
.widenScalarToNextPow2(0)
.clampScalar(1, s32, HasSSE2 ? s64 : s32)
Expand Down Expand Up @@ -709,13 +701,10 @@ bool X86LegalizerInfo::legalizeSITOFP(MachineInstr &MI,
SrcTy.getSizeInBits() == 64) &&
"Unexpected source type for SITOFP in X87 mode.");

const LLT p0 = LLT::pointer(0, MF.getTarget().getPointerSizeInBits(0));
int MemSize = SrcTy.getSizeInBytes();
int StackSlot =
MF.getFrameInfo().CreateStackObject(MemSize, Align(MemSize), false);

auto SlotPointer = MIRBuilder.buildFrameIndex(p0, StackSlot);
MachinePointerInfo PtrInfo = MachinePointerInfo::getFixedStack(MF, StackSlot);
TypeSize MemSize = SrcTy.getSizeInBytes();
MachinePointerInfo PtrInfo;
Align Alignmt = Helper.getStackTemporaryAlignment(SrcTy);
auto SlotPointer = Helper.createStackTemporary(MemSize, Alignmt, PtrInfo);
MachineMemOperand *StoreMMO = MF.getMachineMemOperand(
PtrInfo, MachineMemOperand::MOStore, MemSize, Align(MemSize));

Expand All @@ -738,16 +727,12 @@ bool X86LegalizerInfo::legalizeFPTOSI(MachineInstr &MI,
LegalizerHelper &Helper) const {
MachineFunction &MF = *MI.getMF();
MachineIRBuilder &MIRBuilder = Helper.MIRBuilder;
const LLT p0 = LLT::pointer(0, MF.getTarget().getPointerSizeInBits(0));
auto [Dst, DstTy, Src, SrcTy] = MI.getFirst2RegLLTs();

unsigned MemSize = DstTy.getSizeInBytes();
int StackSlot =
MF.getFrameInfo().CreateStackObject(MemSize, Align(MemSize), false);

auto SlotPointer = MIRBuilder.buildFrameIndex(p0, StackSlot);

MachinePointerInfo PtrInfo = MachinePointerInfo::getFixedStack(MF, StackSlot);
TypeSize MemSize = DstTy.getSizeInBytes();
MachinePointerInfo PtrInfo;
Align Alignmt = Helper.getStackTemporaryAlignment(DstTy);
auto SlotPointer = Helper.createStackTemporary(MemSize, Alignmt, PtrInfo);
MachineMemOperand *StoreMMO = MF.getMachineMemOperand(
PtrInfo, MachineMemOperand::MOStore, MemSize, Align(MemSize));

Expand Down
12 changes: 6 additions & 6 deletions llvm/lib/Target/X86/X86InstrFragments.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,19 +842,19 @@ def X86fldf80 : PatFrag<(ops node:$ptr), (X86fld node:$ptr), [{
def X86fild16 : PatFrag<(ops node:$ptr), (X86fild node:$ptr), [{
return cast<MemIntrinsicSDNode>(N)->getMemoryVT() == MVT::i16;
}]> {
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, 2); }];
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, LLT::scalar(16)); }];
}

def X86fild32 : PatFrag<(ops node:$ptr), (X86fild node:$ptr), [{
return cast<MemIntrinsicSDNode>(N)->getMemoryVT() == MVT::i32;
}]> {
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, 4); }];
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, LLT::scalar(32)); }];
}

def X86fild64 : PatFrag<(ops node:$ptr), (X86fild node:$ptr), [{
return cast<MemIntrinsicSDNode>(N)->getMemoryVT() == MVT::i64;
}]> {
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, 8); }];
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, LLT::scalar(64)); }];
}

def X86fist32 : PatFrag<(ops node:$val, node:$ptr),
Expand All @@ -871,21 +871,21 @@ def X86fp_to_i16mem : PatFrag<(ops node:$val, node:$ptr),
(X86fp_to_mem node:$val, node:$ptr), [{
return cast<MemIntrinsicSDNode>(N)->getMemoryVT() == MVT::i16;
}]> {
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, 2); }];
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, LLT::scalar(16)); }];
Copy link
Contributor

@arsenm arsenm May 14, 2025

Choose a reason for hiding this comment

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

You shouldn't need a custom code predicate for this. There is already a MemoryVT and ScalarMemoryVT field on PatFrag

Copy link
Contributor

@arsenm arsenm May 21, 2025

Choose a reason for hiding this comment

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

Bump

Copy link
Contributor

@e-kud e-kud May 22, 2025

Choose a reason for hiding this comment

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

Hi @arsenm, I don't get the idea. Do you suggest to specify the type of $val and get rid of predicate for both SDAG and GISEL? I've tried to find examples of PatFrag using its memory fields but was unsuccessful.
Otherwise GlobalISel will always try to find its own version of predicate regardless of SDAG implementation.

Copy link
Contributor

@arsenm arsenm May 22, 2025

Choose a reason for hiding this comment

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

No, not the type of val, but the memory type for X86_fp_to_mem. Many of the common custom predicates were moved to specific fields on PatFrag that TableGen recognizes, like the memory alignment to avoid custom predicate code. This is how extending and atomic loads are specified, but the mechanism is supposed to be general.

The example would be how atomic_load_32 and atomic_load_64 work, they each set MemoryVT and wrap another PatFrag for atomic_load, which in turn wraps a PatFrag around ld

}

def X86fp_to_i32mem : PatFrag<(ops node:$val, node:$ptr),
(X86fp_to_mem node:$val, node:$ptr), [{
return cast<MemIntrinsicSDNode>(N)->getMemoryVT() == MVT::i32;
}]> {
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, 4); }];
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, LLT::scalar(32)); }];
}

def X86fp_to_i64mem : PatFrag<(ops node:$val, node:$ptr),
(X86fp_to_mem node:$val, node:$ptr), [{
return cast<MemIntrinsicSDNode>(N)->getMemoryVT() == MVT::i64;
Copy link
Contributor

@e-kud e-kud May 26, 2025

Choose a reason for hiding this comment

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

I expected that we don't need SDAG predicate as well, do we?

Copy link
Contributor

@arsenm arsenm May 27, 2025

Choose a reason for hiding this comment

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

The memory type should replace this

Copy link
Contributor Author

@pawan-nirpal-031 pawan-nirpal-031 May 28, 2025

Choose a reason for hiding this comment

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

I feel that should be done in a separate PR?

Copy link
Contributor

@e-kud e-kud May 28, 2025

Choose a reason for hiding this comment

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

I'm ok with both options.

Copy link
Contributor Author

@pawan-nirpal-031 pawan-nirpal-031 May 29, 2025

Choose a reason for hiding this comment

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

Thank you, I will address this in a separate PR.

}]> {
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, 8); }];
let GISelPredicateCode = [{ return checkMemoryOpSize(MI, LLT::scalar(64)); }];
}

//===----------------------------------------------------------------------===//
Expand Down
Loading