[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
+;
----------------
frasercrmck wrote:
> frasercrmck wrote:
> > deadalnix wrote:
> > > It improved numerous tests, but not this one test case.
> > > 
> > > Other test cases in this file were improved, so this is going in the right direction.
> > Interesting, thanks. So previously I was using a case from `fixed-vectors-int.ll` (`sub`) as I assumed it was the same issue as here - it certainly looked very similar. Come to think of it, maybe that was just for i64 vectors as the scalar is illegal on RV32 so generate SPLAT_VECTOR. I'll see if I can dig in to see what's going on here.
> So, it's to do with bitcasts. I took the `v8i8` case further down which regresses on RV64. First of all, the argument has a different type: i32 or i64 depending on whether we're RV32/RV64. Then we splat:
> 
> ```
>               t6: i64,ch = CopyFromReg t0, Register:i64 %1
>             t12: i8 = truncate t6
>           t14: v8i8 = insert_vector_elt undef:v8i8, t12, Constant:i64<0>
>         t15: v8i8 = vector_shuffle<0,0,0,0,0,0,0,0> t14, undef:v8i8
> ```
> 
> (imagine t6 is typed i32 on RV32)
> 
> So because `v8i8` is the same size as `i64`, on RV64 we combine the eventual BUILD_VECTOR with `DAGCombiner::reduceBuildVecTruncToBitCast`
> 
> ```
> Combining: t29: v8i8 = BUILD_VECTOR t12, undef:i8, undef:i8, undef:i8, undef:i8, undef:i8, undef:i8, undef:i8
>  ... into: t30: v8i8 = bitcast t6
> 
> Combining: t30: v8i8 = bitcast t6
> 
> Combining: t19: v8i8 = vector_shuffle<0,0,0,0,0,0,0,0> t30, undef:v8i8
> ```
> 
> Then the combine of the VECTOR_SHUFFLE fails as it can't handle this pattern, presumably. On RV32 though:
> 
> ```
> Combining: t24: v8i8 = BUILD_VECTOR t12, undef:i8, undef:i8, undef:i8, undef:i8, undef:i8, undef:i8, undef:i8
> 
> Combining: t15: v8i8 = vector_shuffle<0,0,0,0,0,0,0,0> t24, undef:v8i8
>  ... into: t25: v8i8 = BUILD_VECTOR t12, t12, t12, t12, t12, t12, t12, t12
> ```
> 
> Seems like `visitVECTOR_SHUFFLE` of splats needs further updates to account for this bitcast pattern? I'm afraid I don't have the ability to work on this right now.
> 
> I haven't verified but I'd hope this solves most of the remaining cases: the regressions alternate between RV32 and RV64 and it seems to follow the pattern that the vector is the size of the legalized argument.
I think I've solved this with a splat(scalar_to_vector(x), 0) -> build_vector(x,...,x) fold - just dealing with a couple of side regressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127115