[PATCH] D157628: [AArch64][SVE2] Change the cost of extends with S/URHADD to 0

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2047-2056
+// Where SVE2 is enabled, we can combine an add of 1, add & shift right by 1
+// to a single s/urhadd instruction. Some extends can be folded into the
+// instruction and will be 'free', e.g.
+//    %zext1 = zext i8 %a to i16
+//    %zext2 = zext i8 %b to i16
+//    %add1 = add nuw nsw i16 %zext1, 1
+//    %add2 = add nuw nsw i16 %add1, %zext2
----------------
nit: I find the description a bit confusing, how about:

  s/urhadd instructions implements the following pattern, making the extends free:
    %x = (zext i8 -> i16)
    %y = (zext i8 -> i16) + 1)
    trunc i16 (shl (add %x, %y), 1)) -> i8

?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2057
+//
+bool isExtShiftRightAdd(const Instruction *I, const Instruction *Ext, Type *Dst,
+                        Type *Src) {
----------------
nit: Perhaps better to make this a `CastInst`, because it it must be?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2057
+//
+bool isExtShiftRightAdd(const Instruction *I, const Instruction *Ext, Type *Dst,
+                        Type *Src) {
----------------
sdesmalen wrote:
> nit: Perhaps better to make this a `CastInst`, because it it must be?
Can you give `I` a more descriptive name? Or add some documentation above to explain what the relationship between `I` and `Ext` is?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2069
+    // if this is an add.
+    auto *Op = I->getOperand(0) == Ext ? I->getOperand(1) : I->getOperand(0);
+
----------------
This code is puzzling me quite a bit.

Because of how this function is called (`I` is the only user of `Ext`), the only possible inputs we can have is:

  add ( Ext, ... )
  add ( ..., Ext )

So if `Op1` is not a constant, then the input must be `add (..., Ext)`. Here you're asking the question if `... == Ext`, which we know is `false`, after which `Op` is assigned `...`.  It then changes the meaning of `I`, at which point I'm a bit lost :) It's a lot easier to use the Patterns from PatternMatch.h for this, that way you can do things like:

  if (match(I, m_c_Add(m_Specific(Ext),
                       m_c_Add(m_ZExt(m_Value(V)), m_SpecificInt(1)))))

which also handles the commutativity of the add.

Note that instead of directly matching for m_ZExt, you could match `m_UnOp` and have another check to see that it's either `ZExt` (if `Ext` is a `ZExt`), or a SExt (if `Ext` is a `SExt`).


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2086
+  // The add should only have one user, a right shift of 1.
+  auto *Add = cast<Instruction>(*I->user_begin());
+  if (Add->getOpcode() != Instruction::Add || !Add->hasOneUser())
----------------
Is it worth first checking if this add has a single user which is a `LShr`, which itself has a single user that's a `Trunc`? That way you avoid having to match the whole expression, only to find out that the user of the `add(add(..), Ext)` isn't used by a `LShr`.

It probably makes it a lot easier to match the entire pattern once you know you have a `Trunc(LShr(Add(..), ..))` expression on your hands, when you take my suggestion above to use the `m_<patterns>` from PatternMatch.h.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157628/new/

https://reviews.llvm.org/D157628