[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