[PATCH] D127115: [RFC][DAGCombine] Make sure combined nodes are added back to the worklist in topological order.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vxor-vp.ll:166
+; RV32-NEXT:    vxor.vv v8, v8, v10, v0.t
+; RV32-NEXT:    ret
+;
----------------
RKSimon wrote:
> frasercrmck wrote:
> > frasercrmck wrote:
> > > RKSimon wrote:
> > > > frasercrmck wrote:
> > > > > RKSimon wrote:
> > > > > > @craig.topper Might all of these vrgather regressions be due to a missing combine for splat cases?
> > > > > Something like that. I see that we have the initial pattern:
> > > > > ```
> > > > >           t9: v2i64 = insert_vector_elt undef:v2i64, Constant:i64<-1>, Constant:i64<0>
> > > > >         t10: v2i64 = vector_shuffle<0,0> t9, undef:v2i64
> > > > > ```
> > > > > 
> > > > > on RV64 that's combined:
> > > > > ```
> > > > > Combining: t9: v2i64 = insert_vector_elt undef:v2i64, Constant:i64<-1>, Constant:i64<0>
> > > > >  ... into: t14: v2i64 = BUILD_VECTOR Constant:i64<-1>, undef:i64
> > > > > 
> > > > > Combining: t14: v2i64 = BUILD_VECTOR Constant:i64<-1>, undef:i64
> > > > > 
> > > > > Combining: t10: v2i64 = vector_shuffle<0,0> t14, undef:v2i64
> > > > >  ... into: t15: v2i64 = BUILD_VECTOR Constant:i64<-1>, Constant:i64<-1>
> > > > > 
> > > > >         t15: v2i64 = BUILD_VECTOR Constant:i64<-1>, Constant:i64<-1>
> > > > >       t11: v2i64 = sub t5, t15
> > > > > ```
> > > > > 
> > > > > On RV32 it's:
> > > > > ```
> > > > > Combining: t8: v2i64 = insert_vector_elt undef:v2i64, Constant:i64<-1>, Constant:i32<0>
> > > > >  ... into: t15: v2i64 = BUILD_VECTOR Constant:i64<-1>, undef:i64
> > > > > 
> > > > > Combining: t15: v2i64 = BUILD_VECTOR Constant:i64<-1>, undef:i64
> > > > >  ... into: t16: v2i64 = splat_vector Constant:i64<-1>
> > > > > 
> > > > > Combining: t16: v2i64 = splat_vector Constant:i64<-1>
> > > > > 
> > > > > Combining: t9: v2i64 = vector_shuffle<0,0> t16, undef:v2i64
> > > > > 
> > > > >           t16: v2i64 = splat_vector Constant:i64<-1>
> > > > >         t9: v2i64 = vector_shuffle<0,0> t16, undef:v2i64
> > > > >       t10: v2i64 = sub t5, t9
> > > > > ```
> > > > > 
> > > > > I'm not able to say what //exactly// is going wrong, though. We're definitely missing combines for shuffles of splats, but do we really want to see `splat_vector` for fixed-length vectors here?
> > > > Something you might consider is just use SelectionDAG::getSplatSourceVector / isSplatValue - that way you aren't having to handle specific splat patterns.
> > > Perhaps yeah. I should've said, though, that none of this is happening in RISCV-specific combines. This seems to purely be `visitBUILD_VECTOR` doing the BV->splat combine, and `visitVECTOR_SHUFFLE` working on BV but not splats.
> > The only reason it "works" before this patch is because we visit the `vector_shuffle` before the newly-created `BUILD_VECTOR` so no splat is created:
> > 
> > ```
> > Combining: t8: v2i64 = insert_vector_elt undef:v2i64, Constant:i64<-1>, Constant:i32<0>
> >  ... into: t15: v2i64 = BUILD_VECTOR Constant:i64<-1>, undef:i64
> > 
> > Combining: t9: v2i64 = vector_shuffle<0,0> t15, undef:v2i64
> >  ... into: t16: v2i64 = BUILD_VECTOR Constant:i64<-1>, Constant:i64<-1>
> > ```
> > 
> > I suppose that suggests we should enhance `visitVECTOR_SHUFFLE` to handle splats after all.
> Cheers - looking at it now
@deadalnix Please can you rebase on rGa083f3caa135 to see if this catches these cases?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127115