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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck 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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127115