[PATCH] D140200: [AArch64][InstCombine] Fuse ADD+MUL and SUB+MUL AArch64 instrinsics

Matt Devereau via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 20 06:43:22 PST 2022


MattDevereau marked an inline comment as done.
MattDevereau added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1224
+    return FMSB;
   return instCombineSVEVectorBinOp(IC, II);
 }
----------------
sdesmalen wrote:
> nit: Can you add a comment saying that there is no integer version of nmsb for the equivalent integer case of the above?
> 
> (is there a negative test available for that case?)
I'm personally of the opinion that comments describing the absence of things are bloat but I'm happy to put it in.

Regarding the test, I'm not sure what it would be beyond a `sub(mul)` with no transformation. Without the context of other tests surrounding it, I'm not sure it would make much sense.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-muladdsub.ll:516
+
+define dso_local <vscale x 8 x half> @neg_combine_fnmsb_two_fmul_uses(<vscale x 16 x i1> %p, <vscale x 8 x half> %a, <vscale x 8 x half> %b, <vscale x 8 x half> %c) local_unnamed_addr #0 {
+; CHECK-LABEL: @neg_combine_fnmsb_two_fmul_uses(
----------------
sdesmalen wrote:
> I don't necessarily think you need to repeat these negative tests for all the fmla/fmad/fmls/.. combinations (same for the tests above (predicate not matching, not the right fast-math flags, etc). Maybe you can have all positive tests firsts, and then all the negative tests for just the `fmla`, since they should directly translate to the other mul-add intrinsics.
Sure, these were just low overhead copy/pasted tests and the repetition is unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140200



More information about the llvm-commits mailing list