Skip to content

[RISC-V][GISEL] Select G_BITCAST for scalable vectors #101486

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

[RISC-V][GISEL] Select G_BITCAST for scalable vectors #101486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cea2c3e
bitcast as copy?
jiahanxie353 Aug 1, 2024
7d9d4f2
add instr select tests for load and store
jiahanxie353 Sep 23, 2024
907dbb3
pre instr select lower to force load/store of pointers to xlen integers
jiahanxie353 Sep 23, 2024
96e23b4
update tests to remove llvm ir and update test checks
jiahanxie353 Sep 24, 2024
057c06a
use replacePtrWithInt for G_LOAD
jiahanxie353 Sep 24, 2024
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
44 changes: 38 additions & 6 deletions llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,7 @@ bool RISCVInstructionSelector::select(MachineInstr &MI) {
case TargetOpcode::G_INTTOPTR:
case TargetOpcode::G_TRUNC:
case TargetOpcode::G_FREEZE:
case TargetOpcode::G_BITCAST:
Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to need G_BITCAST instruction-select tests due to this line

return selectCopy(MI, MRI);
case TargetOpcode::G_CONSTANT: {
Register DstReg = MI.getOperand(0).getReg();
Expand Down Expand Up @@ -757,13 +758,21 @@ bool RISCVInstructionSelector::replacePtrWithInt(MachineOperand &Op,
MachineIRBuilder &MIB,
MachineRegisterInfo &MRI) {
Register PtrReg = Op.getReg();
assert(MRI.getType(PtrReg).isPointer() && "Operand is not a pointer!");

const LLT PtrTy = MRI.getType(PtrReg);
const LLT sXLen = LLT::scalar(STI.getXLen());
auto PtrToInt = MIB.buildPtrToInt(sXLen, PtrReg);
MRI.setRegBank(PtrToInt.getReg(0), RBI.getRegBank(RISCV::GPRBRegBankID));
Op.setReg(PtrToInt.getReg(0));
return select(*PtrToInt);
if (PtrTy.isPointer()) {
auto PtrToInt = MIB.buildPtrToInt(sXLen, PtrReg);
MRI.setRegBank(PtrToInt.getReg(0), RBI.getRegBank(RISCV::GPRBRegBankID));
Op.setReg(PtrToInt.getReg(0));
return select(*PtrToInt);
}
assert(PtrTy.isPointerVector() &&
"Operand must be a pointer of a vector of pointers");
assert(PtrTy.isScalableVector() &&
"Currently only working for scalable vector of pointers now");
MRI.setType(PtrReg, LLT::scalable_vector(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a helper function called replacePtrWithInt. Maybe you can expand that to support vectors instead of doing it here in the code manually?

Not sure if makes sense to inplace update and return true, instead of building a new instruction like G_PTRTOINT. I didn't do it because there is no generic opcode for it and we have to go through extract_elment and apply each element with G_PTRTOINT which feels very cumbersome.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like AArch64 changes the type without building a PtrToInt. They say it works because all users have been selected already (bottom up selection) so the type does not matter for them. I think we can do the same thing, which is what you have done here.

I wonder if we really needed to build the PtrToInt above for the same reason. I can take a look at that in a follow up patch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the vector and non-vector code do something wildly different they should not be in the same function.

PtrTy.getElementCount().getKnownMinValue(), sXLen));
return true;
}

void RISCVInstructionSelector::preISelLower(MachineInstr &MI,
Expand All @@ -785,6 +794,29 @@ void RISCVInstructionSelector::preISelLower(MachineInstr &MI,
replacePtrWithInt(MI.getOperand(1), MIB, MRI);
MI.setDesc(TII.get(TargetOpcode::G_AND));
MRI.setType(DstReg, sXLen);
break;
}
case TargetOpcode::G_LOAD: {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a helper function called replacePtrWithInt. Maybe you can expand that to support vectors instead of doing it here in the code manually?

Register DstReg = MI.getOperand(0).getReg();
const LLT DstTy = MRI.getType(DstReg);
if (!DstTy.isPointerVector() &&
"Destination register that's not a vector of pointers doesn't need to "
"go through preISelLower")
break;
replacePtrWithInt(MI.getOperand(0), MIB, MRI);
break;
}
case TargetOpcode::G_STORE: {
MachineOperand &SrcOp = MI.getOperand(0);
const LLT SrcTy = MRI.getType(SrcOp.getReg());
if (!SrcTy.isPointerVector() &&
"Source register that's not a vector of pointers doesn't need to go "
"through preISelLower")
break;
const LLT sXLen = LLT::scalar(STI.getXLen());
auto Copy = MIB.buildCopy(LLT::scalable_vector(SrcTy.getElementCount().getKnownMinValue(), sXLen), SrcOp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we do a copy instead of changing the store's vector pointer element type to vector of sXLen type?

Copy link
Collaborator

@topperc topperc Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the pointer has p0 type. we need to change the type of the data to store, but that type is owned by the instruction that produces the data. so you can't change it. if you were to change it that instruction would fail instruction selection when we visit it.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: clang format

Register NewSrc = Copy.getReg(0);
SrcOp.setReg(NewSrc);
}
}
}
Expand Down
Loading
Loading