Details
- Reviewers
paulwalker-arm
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
15427–15430 | What's the value provided here? because I only see a single use. | |
15435 | LLVM coding standard prefers early exits, so this should be if (Op.getOpcode() == AArch64ISD::CSEL) return None; | |
15441 | Unnecessary {} for single line block. Also applies to the next if block. | |
15478 | I guess Dave will say OpLHS here? Likewise in getCSETCondCode. | |
15486–15487 | Perhaps a silly question but this suggests ADC l r (CMP (CSET HS carry) 1)) == ADC l r (CMP (CSET HI carry) 1)) which at first glance doesn't look correct. Looking at the test changes I only see instances of HS and LO. Sure HS and HI both set the carry flag but for the HI case that's not enough for CSET to return 1. Am I missing something? | |
15493 | This seems weird. You either know CsetOp is a CSET or you don't. If you do then you know exactly which operand represents the carry? | |
15499 | Does just Op->getVTList() work here? | |
18675–18680 | How safe is this? i.e. do all the transformations relevant to ADD and SUB also apply to these nodes. My cursory glance suggests not. Even for the cases where the opcode is checked it seems inefficient to call a bunch of try... functions we know ahead of time are useless. There's an argument performAddSubCombine should be sliced in two given there's not much overlap between the ADD and SUB cases. But that's not your problem so I guess my question is why not just have your new opcodes call directly into foldOverflowCheck? Also the ADDS and SUBS cases look redundant? |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
15435 | Or rather if (Op.getOpcode() != AArch64ISD::CSEL) return None; :) |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
15427–15430 | It is used in later patches |
Remove erroneous transformation of (ADC l r (CMP CSET HI carry) 1)) and (SBC l r (CMP (CSET LS carry) 1))
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
15454–15457 | Given these are related perhaps merge these two if blocks? |
We are noticing a clang crash after this commit: https://github.com/llvm/llvm-project/issues/55510
What's the value provided here? because I only see a single use.