This seems to mostly work, but I'm not sure I'm doing it at the right layer. Maybe we should be trying to match this before shuffle lowering?
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Generally, looks good. I have some comments on the format of the code, which could be a bit cleaner on the depths of the ifs and commoning up the checks.
cheers,
--renato
lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
9240 | Why v2i32 only? I can see from your tests that this also matches i8, so I'm suspecting this is already extended by the selection dag before getting here? Also, VPADD works with floats, too. | |
9242 | Better to negate this if and move return SDValue() here. | |
9256 | This if is almost identical to the one above. I'd common them up in a local static function. | |
9260 | VPADDL works on D and Q regs, not Q only. | |
9261 | Same comment here, better to negate this if and return SDValue(). |
lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
9240 | This conditional is kind of awkward; I really want to just look for VUZP, but there is no v2i32 VUZP. (The float vpadd isn't quite the same thing, but I guess I can look into it.) | |
9260 | Yes... but in that case the type of N00 would be a 32-bit vector, and the whole thing kind of explodes due to the way legalization works. I'll include a testcase for reference. |
lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
9663 | This looks weird. As much as I have long methods and nested ifs, the interaction between Early and Late increases the coupling between them and make calling code more complex. (for example, you have an early exit on Early for later match on Later). Adding an if block in AddCombineToVPADDL with an return would be much cleaner, if the condition is clear and concise, which should be the case, with your new helper. |
lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
9663 | They aren't really coupled like that; when I say "we'll match it to vpadd later", I mean, way later, like after the BUILD_VECTOR is lowered by legalization. Maybe I could rename "Early" and "Late" to be a bit more clear... Also, my eventual intent is to kill off AddCombineToVPADDLEarly; I'm just leaving it around to avoid regressing the fromExtendingExtractVectorElt_i8 and fromExtendingExtractVectorElt_i16 cases, which get lowered in stupid ways otherwise. |
lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
9240 | Oh, I see! ok. | |
9663 | Right, I see what you mean. If you rename to what they match (instead of early/late), they'll become two different calls anyway, just like the rest of them in this function. It'd also good to add a FIXME on the current Early one, so anyone looking will know. |
Why v2i32 only? I can see from your tests that this also matches i8, so I'm suspecting this is already extended by the selection dag before getting here?
Also, VPADD works with floats, too.