[PATCH] D111638: [AArch64][SVE] Combine predicated FMUL/FADD into FMA

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:718
+  if (!match(AddOp1, m_Intrinsic<Intrinsic::aarch64_sve_fmul>()))
+    return None;
+
----------------
MattDevereau wrote:
> MattDevereau wrote:
> > peterwaller-arm wrote:
> > > peterwaller-arm wrote:
> > > > I'd expect this all to look simpler, please have a go at simplifying. I think you can drop the matching logic above and instead add a condition that checks the AddOp1's predicate matches the Add's predicate.
> > > > 
> > > > One issue I have is that both the swap and m_SVEFAdd as it currently is serve the purpose of testing both operand orders. I think it's important for clarity to only do this once.
> > > If you want to use the match syntax above I think you can also bind the fmul with `m_And(m_Value(FMul), m_SVEFMul(m_Deferred(p), m_Value(a), m_Value(b))` and then you could grab `FMul` and `c` simultaneously with the existing logic.
> > I've opted to check AddOp1's predicate matches the Add's predicate
> Was this what you meant?
> 
> ```
>   if (!match(&II, m_SVEFAdd(m_Value(p),
>                             m_And(m_Value(FMul), m_SVEFMul(m_Deferred(p), m_Value(a), m_Value(b))),
>                             m_Value(c)))){
>     return None;
>   }
> ```
> FMul isn't matching in this expression
Sorry, I think I meant match(X, m_CombineAnd(A,B)) which means to match both X on A and X on B. I erroneously wrote m_And(A,B) which means to match X on the expression `A && B`.

See these two:

https://github.com/llvm/llvm-project/blob/3efd2a0bec0298d804f274fcc10ea14431b61de1/llvm/include/llvm/IR/PatternMatch.h#L1122-L1126

https://github.com/llvm/llvm-project/blob/3efd2a0bec0298d804f274fcc10ea14431b61de1/llvm/include/llvm/IR/PatternMatch.h#L194-L218



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:724
+  llvm::FastMathFlags flags = II.getFastMathFlags();
+  flags &= FMulInst->getFastMathFlags();
+
----------------
MattDevereau wrote:
> peterwaller-arm wrote:
> > peterwaller-arm wrote:
> > > It seems to me that a check is needed if the fast math flags contain 'contract', and if not, bailout.
> > Please also check that the flags are equal instead of taking their intersection. The fadd might plausibly have a flag which allows more aggressive optimization and contracting it in this way may prevent those optimizations from taking place.
> I'm not sure what you mean here. Since flags is its own fresh variable isnt the intersection the same as comparing if the flags are equal? The original flags on the fadd intrinsic will be preserved
When I wrote 'contracting' I think I was thinking 'intersecting'.

My suggestion is to do `if (flags1 == flags2) return None;`, which cannot be the same as doing an intersection -- in my proposed case the optimization would not take place. Intersection implies constructing a new set of flags which is different from those flags sets on the input. Also not sure what you mean by 'the original flags on the fadd intrinsic will be preserved' -- we're expecting that this optimization will replace the fadd with the newly constructed fmul, so the fadd is going to be erased.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111638