[PATCH] D142656: [SVE][codegen] Add pattern for SVE multiply-add accumulate

[Sushant Gokhale via Phabricator via llvm-commits]

sushgokh added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:350
+  unsigned nUses = 0;
+  for(SDNode* use: Op1->uses())
+  {
----------------
SjoerdMeijer wrote:
> This doesn't do what you think it does, I think.
> This returns the uses of this node in the graph, it doesn't return the def-uses.
> 
> So the question is for which test-case you need this and why?
->uses() indeed returns users of the node


================
Comment at: llvm/test/CodeGen/AArch64/sve-multiply-add-accumulate.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=aarch64-unknown-linux-gnu -mattr=+sve < %s | FileCheck %s
+
----------------
david-arm wrote:
> It would be really nice if you could pre-commit these tests without any code changes so that we can see the diff in this patch?
Added a patch [[ https://reviews.llvm.org/D142998 | D142998  ]].

Once thats commited, will update this patch


================
Comment at: llvm/test/CodeGen/AArch64/sve-multiply-add-accumulate.ll:80
+; MAD result can be computed in register for 'a'
+define <vscale x 8 x i16> @muladd_i16_test4(<vscale x 8 x i16> %a, <vscale x 8 x i16> %b)
+; CHECK-LABEL: muladd_i16_test4:
----------------
david-arm wrote:
> Without seeing the code for these tests before your patch it's hard to know what's changed and whether this is an improvement or not. I copied these tests and ran them manually without your patch and to be honest the code for this function isn't obviously better with the mad:
> 
>   muladd_i16_test4:                       // @muladd_i16_test4
>         ptrue   p0.h
>         mul     z0.h, p0/m, z0.h, z1.h
>         add     z0.h, z0.h, #200                // =0xc8
>         mul     z0.h, p0/m, z0.h, z1.h
>         ret
> 
> I think it's because the add is able to combine the constant into the instruction, whereas mad cannot. Having said that, I can believe that for non-constant cases this might be a win. It might be a good idea to include some tests for non-constants too?
Have added a patch for tests. Refer D142998


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142656