Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
One nit plus a suggestion but otherwise the patch looks good to me. There are several regressions though, presumably caused by my request to split the original patch up. I'm not qualified to say how important these test cases are from a performance point of view but given the number of regressions I think it's worth creating a patch series (via Depends On) so the patches can be reviewed in isolation but pushed together. Sorry if this is messing you around.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
3365 | Unnecessary {} for single line body. | |
3369–3390 | Up to you but I think these are better passed in a parameters so you end up with: case ISD::ADDCARRY: return lowerADDSUBCARRY(Op, DAG, AArch64ISD::ADCS, false /*unsigned*/); case ISD::SUBCARRY: return lowerADDSUBCARRY(Op, DAG, AArch64ISD::SBCS, false /*unsigned*/); case ISD::SADDO_CARRY: return lowerADDSUBCARRY(Op, DAG, AArch64ISD::ADCS, true /*signed*/); case ISD::SSUBO_CARRY: return lowerADDSUBCARRY(Op, DAG, AArch64ISD::SBCS, true /*signed*/); |
Yeah, we would need to make sure that the regressions are fixed either soon after, or preferably before this. 128 bit arithmetic is fairly rare, but can come up from certain disciplines. (Sometimes it's possible to test the optimizations independently with other code patterns. Sometimes that's not possible though).
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
3365 | Is this just checking for i32 or i64? It might be better to be explicit about that. | |
3392 | OpLhs -> OpLHS |
Update revision according to feedback
- Remove unecessary {} for single-line body.
- Pass in Opcode and IsSigned as parameters
- Explicitly check for MVT::i32 and MVT::i64
- Capitalisation of OpLHS and OpRHS
According to https://github.com/llvm/llvm-project/issues/55253, this causes a miscompile.
Unnecessary {} for single line body.