[PATCH] D146494: [X86] Combine constant vector inputs for FMA

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:54037
+    if (Subtarget.hasAVX2() && V.hasOneUse() &&
+        ISD::isConstantSplatVector(V.getNode(), SplatValue)) {
+      SmallVector<SDValue, 8> Ops;
----------------
RKSimon wrote:
> RKSimon wrote:
> > pengfei wrote:
> > > Why just limit to splat constant?
> > Also - why the AVX2 and hasOneUse limits?
> Sorry - the hasOneUse is probably necessary to stop ping-ponging between negated constants
> Sorry - the hasOneUse is probably necessary to stop ping-ponging between negated constants

That should be commented explicitly as these subtle checks to prevent infinite loops can easily be removed accidentally.

Also why AVX2 still?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:54037
+    if (Subtarget.hasAVX2() && V.hasOneUse() &&
+        ISD::isConstantSplatVector(V.getNode(), SplatValue)) {
+      SmallVector<SDValue, 8> Ops;
----------------
goldstein.w.n wrote:
> RKSimon wrote:
> > RKSimon wrote:
> > > pengfei wrote:
> > > > Why just limit to splat constant?
> > > Also - why the AVX2 and hasOneUse limits?
> > Sorry - the hasOneUse is probably necessary to stop ping-ponging between negated constants
> > Sorry - the hasOneUse is probably necessary to stop ping-ponging between negated constants
> 
> That should be commented explicitly as these subtle checks to prevent infinite loops can easily be removed accidentally.
> 
> Also why AVX2 still?
> Also - why the AVX2 and hasOneUse limits?




================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:54037
+    if (Subtarget.hasAVX2() && V.hasOneUse() &&
+        ISD::isConstantSplatVector(V.getNode(), SplatValue)) {
+      SmallVector<SDValue, 8> Ops;
----------------
goldstein.w.n wrote:
> goldstein.w.n wrote:
> > RKSimon wrote:
> > > RKSimon wrote:
> > > > pengfei wrote:
> > > > > Why just limit to splat constant?
> > > > Also - why the AVX2 and hasOneUse limits?
> > > Sorry - the hasOneUse is probably necessary to stop ping-ponging between negated constants
> > > Sorry - the hasOneUse is probably necessary to stop ping-ponging between negated constants
> > 
> > That should be commented explicitly as these subtle checks to prevent infinite loops can easily be removed accidentally.
> > 
> > Also why AVX2 still?
> > Also - why the AVX2 and hasOneUse limits?
> 
> 
> Why just limit to splat constant?

Should be match on ImmConstant no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146494