This information is not preserved in MIR today. So this patch adds
information to RISCVMachineFunctionInfo when the vreg is created for
the argument.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Playing around with this on the GCC torture suite, I'm seeing a number of small codegen improvements, no miscompiles, and no instruction count regressions worth worrying about (pr67037 gets a few more instructions added than are removed, seems to just be due to slightly different BB structure due to different regalloc choices).
Comment Actions
This looks reasonable to me, but I'm not knowledgeable enough of the argument lowering to know if this scheme is sound. I'll defer to others on the review.
| llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp | ||
|---|---|---|
| 321 | These declarations can be pulled out of the loop. | |
| llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
|---|---|---|
| 10711 | One thought here. If this logic is sound, then we could use a similar approach to prove known bits for the RegisterSDNode representing the argument location. This would allow SDAG to eliminate some extends, but more importantly, might allow us to do this in a place more likely to expose any unsoundness in a way it gets caught. Particularly, if we can do that in target independent code. Looking at the calling code, I think this basically translates to putting an AssertSext after the CopyFromReg? Actually, it looks like the caller already does this for us based on the isZExt and isSExt flags. I suspect you can rewrite this code as if not split and is sext from the flags. Not sure mind you, just suspect. | |
| llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
|---|---|---|
| 10713–10717 | Don't see why you need to look at the underlying IR here instead of just relying on the argument flags | |
| llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp | ||
| 9 | It feels wrong to me that you would need to optimize these after selection but I guess I don't know why you are seeing these | |
| llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
|---|---|---|
| 10711 |
That is correct. We already do exactly that. The changed test in this patch hit the depth limit on computeKnownBits in SelectionDAG or it would have been optimized.
That wouldn't cover the i8 and i16 zext case. And I don't know how the sext in the flags behaves if someone puts it on an i64 that doesn't need to be promoted. clang wouldn't do that, but that's not the only source of IR. | |
| 10713–10717 | If I don't look at the original IR, then I would do the wrong thing if someone put a signext attribute on an i33 argument. I also wouldn't be able to handle zeroext for i16 and i8. | |
| llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp | ||
|---|---|---|
| 9 | Some of them are from SelectionDAG's depth limit in simplifyDemandedBits/computeKnownBits/computeNumSignBits. Some of them are because this handles phi loops and SelectionDAG doesn't. Not sure if there are other reasons. | |
| llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp | ||
|---|---|---|
| 9 | This pass also converts some instructions to their W forms if it would remove a sext.w later. SelectionDAG doesn't do that. | |
| llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
|---|---|---|
| 10711 | On your last point, I don't think you're correct. Here is some pseudo code: if (!In.isSplit() && LocVT == i64) {
auto ValVT = VA.getValVT()
if (In.isSExt() && ValVT in {MVT::is32, i16, i8}
// set sext32 flag
} else if (In.isZExt() && ValVT in {i16, i8}) {
//set sext32 flag
}
}This doesn't handle odd sized integers as neatly, but do we care? To be clear, arguing to be pedantic and to understand. I am not objecting to the approach taken. | |
| llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
|---|---|---|
| 10711 | ValVT is always the same as LocVT. The promotion to i64 happens in SelectionDAGBuilder before the setting of ValVT and LocVT happens. | |
| llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
|---|---|---|
| 10711 | AArch64 also has code to recover the original VT from the IR for (unsigned i = 0; i != NumArgs; ++i) {
MVT ValVT = Ins[i].VT;
if (Ins[i].isOrigArg()) {
std::advance(CurOrigArg, Ins[i].getOrigArgIndex() - CurArgIdx);
CurArgIdx = Ins[i].getOrigArgIndex();
// Get type of the original argument.
EVT ActualVT = getValueType(DAG.getDataLayout(), CurOrigArg->getType(),
/*AllowUnknown*/ true);
MVT ActualMVT = ActualVT.isSimple() ? ActualVT.getSimpleVT() : MVT::Other;
// If ActualMVT is i1/i8/i16, we should set LocVT to i8/i8/i16.
if (ActualMVT == MVT::i1 || ActualMVT == MVT::i8)
ValVT = MVT::i8;
else if (ActualMVT == MVT::i16)
ValVT = MVT::i16;
} | |
| llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
|---|---|---|
| 10711 | Ah, agreed. I'd missed the order of operation happening here. | |
One thought here.
If this logic is sound, then we could use a similar approach to prove known bits for the RegisterSDNode representing the argument location. This would allow SDAG to eliminate some extends, but more importantly, might allow us to do this in a place more likely to expose any unsoundness in a way it gets caught. Particularly, if we can do that in target independent code.
Looking at the calling code, I think this basically translates to putting an AssertSext after the CopyFromReg? Actually, it looks like the caller already does this for us based on the isZExt and isSExt flags.
I suspect you can rewrite this code as if not split and is sext from the flags. Not sure mind you, just suspect.