These kind of go together, since fcmp expansion generates boolean operations.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
|---|---|---|
| 7563 | It seems like this code is missing a guard for VT.isScalableVector() (same for the aarch64_sve_whilelo case below) | |
| llvm/lib/Target/AArch64/SVEInstrFormats.td | ||
| 1369 | Do we want to create a SVE_2_Op_<..>_Pat or something for these? There will be other instructions where this would be useful as well. nit: please reverse the order so it matches the patterns straight above it. | |
| llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
|---|---|---|
| 7563 | Not sure what you think needs to be handled; non-scalable i1 vectors aren't legal. | |
| llvm/lib/Target/AArch64/SVEInstrFormats.td | ||
| 1369 | I'm not sure what that would look like. Are you proposing something like SVE_2_Op_PTRUE_H_Pat/SVE_2_Op_PTRUE_S_Pat/etc.? | |
| llvm/lib/Target/AArch64/SVEInstrFormats.td | ||
|---|---|---|
| 1369 | There's something similar in D71712: class SVE_2_Op_AllActive_Pat<ValueType vtd, SDPatternOperator op, ValueType vt1,
ValueType vt2, Instruction inst, Instruction ptrue>
: Pat<(vtd (op vt1:$Op1, vt2:$Op2)),
(inst (ptrue 31), $Op1, $Op2)>;Seems like a good fit, but I'm not sure... | |
| llvm/lib/Target/AArch64/SVEInstrFormats.td | ||
|---|---|---|
| 1369 | So this would be written like the following? def : SVE_2_Op_AllActive_Pat<nxv2i1, op_nopred, nxv2i1, nxv2i1,
!cast<Instruction>(NAME), PTRUE_D>;It doesn't seem much better than what I've written, but okay, I guess. | |
LGTM!
| llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
|---|---|---|
| 7563 | Yes, of course! Thanks for adding that comment. | |
| llvm/lib/Target/AArch64/SVEInstrFormats.td | ||
| 1369 | Cheers, FWIW this makes it a bit more readable to me (especially in conjunction with the use of SVE_3_Op_Pat above). | |
| 1371 | This file doesn't seem too strict on the 80char limit, so I think having this on the same line would make it more readable. | |
| llvm/lib/Target/AArch64/SVEInstrFormats.td | ||
|---|---|---|
| 1371 | It's not strict, yes, but I think going over 100 columns is probably not a good idea. | |
clang-format: please reformat the code
- for (auto VT : - { MVT::nxv2f16, MVT::nxv4f16, MVT::nxv8f16, MVT::nxv2f32, MVT::nxv4f32, - MVT::nxv2f64 }) { + for (auto VT : {MVT::nxv2f16, MVT::nxv4f16, MVT::nxv8f16, MVT::nxv2f32, + MVT::nxv4f32, MVT::nxv2f64}) {