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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1224
+    return FMSB;
   return instCombineSVEVectorBinOp(IC, II);
 }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140200