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

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1224
+    return FMSB;
   return instCombineSVEVectorBinOp(IC, II);
 }
----------------
sdesmalen wrote:
> MattDevereau wrote:
> > 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.
> > 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.
> The 'no transformation' would be the point. i.e. there is a positive test for the floating-point case that translates to fnmsb, and we'd have a negative test for the integer form because there is no equivalent integer instruction (it just covers that we don't make any wrong assumptions)
> 
> > I'm personally of the opinion that comments describing the absence of things are bloat but I'm happy to put it in.
> Fair enough, I'm happy for you to omit adding a comment here, if we have a test that covers it with a comment that explains the need for the test. That should guard it from adding an invalid transform in the future.
I've added a test for this with a comment describing why it's there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140200