[PATCH] D113095: Combine FADD and FMUL aarch64 intrinsics to FMLA

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:701
+  // fold (fadd p a (fmul p b c)) -> (fma p a b c)
+  Value *p = II.getOperand(0);
+  Value *a = II.getOperand(1);
----------------
MattDevereau wrote:
> peterwaller-arm wrote:
> > MattDevereau wrote:
> > > paulwalker-arm wrote:
> > > > These should be upper case to match LLVM styling and be consistent with the other names using within this patch.
> > > I'm not 100% sure what you mean by "these" being consistent with the other names
> > > 
> > > p -> P
> > > a -> A
> > > b -> B
> > > c -> C
> > > FMul -> FMUL?
> > > 
> > Paul is referring to this section of the style guide: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> > 
> > I think `FMul` is in keeping with the surroundings as it is. Your other suggestions look fine to me in the context, or you could name them more thoroughly (Pred/Predicate, OpA, OpB, OpC) for example.
> I've changed FMul to FMUL to stay in line with FMLA further down in the function
FMLA is an acronym so warrants full capitalization, FMul is more like PTest, and you can find other examples of things being called Mul. It's camel case, so I preferred it as you had it before (there are other examples in the file of 'Mul' for example).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113095