[PATCH] D27779: [ARM] More aggressive matching for vpadd and vpaddl.

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9219
+  if ((N0.getOpcode() == ARMISD::VUZP ||
+      (N0.getOpcode() == ARMISD::VTRN && N0.getValueType() == MVT::v2i32)) &&
+      N0.getNode() == N1.getNode() &&
----------------
efriedma wrote:
> rengolin wrote:
> > 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 `float`s, too.
> 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.)
Oh, I see! ok.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9663
   // Attempt to create vpaddl for this add.
-  if (SDValue Result = AddCombineToVPADDL(N, N0, N1, DCI, Subtarget))
+  if (SDValue Result = AddCombineToVPADDLEarly(N, N0, N1, DCI, Subtarget))
+    return Result;
----------------
efriedma wrote:
> rengolin wrote:
> > This looks weird. As much as I have long methods and nested `if`s, 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.
> 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D27779