[PATCH] D139637: [AArch64][SVE][ISel] Combine dup of load to replicating load

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/test/tools/llvm-mca/AArch64/Neoverse/N2-sve-instructions.s:4475
 # CHECK-NEXT:  1      6     0.50    *                   ld1h	{ z5.h }, p3/z, [x17, x16, lsl #1]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rb	{ z0.b }, p0/z, [x0]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rb	{ z0.d }, p0/z, [x0]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rb	{ z0.h }, p0/z, [x0]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rb	{ z0.s }, p0/z, [x0]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rb	{ z31.b }, p7/z, [sp, #63]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rb	{ z31.d }, p7/z, [sp, #63]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rb	{ z31.h }, p7/z, [sp, #63]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rb	{ z31.s }, p7/z, [sp, #63]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rd	{ z0.d }, p0/z, [x0]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rd	{ z31.d }, p7/z, [sp, #504]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rh	{ z0.d }, p0/z, [x0]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rh	{ z0.h }, p0/z, [x0]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rh	{ z0.s }, p0/z, [x0]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rh	{ z31.d }, p7/z, [sp, #126]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rh	{ z31.h }, p7/z, [sp, #126]
-# CHECK-NEXT:  1      6     0.33    *             U     ld1rh	{ z31.s }, p7/z, [sp, #126]
+# CHECK-NEXT:  1      6     0.33    *                   ld1rb	{ z0.b }, p0/z, [x0]
+# CHECK-NEXT:  1      6     0.33    *                   ld1rb	{ z0.d }, p0/z, [x0]
----------------
paulwalker-arm wrote:
> dmgreen wrote:
> > Allen wrote:
> > > dmgreen wrote:
> > > > Allen wrote:
> > > > > Excuse me, I'm curious why the above changes affect the **SideEffects attribute** here. Would you give me some guidance? Thank you.
> > > > The way that hasSideEffects for an instruction works is that:
> > > >  - It defaults to true.
> > > >  - It gets set to false if there is a tablegen pattern that generated that instruction (I believe it might needs to be a single instruction generated by the pattern).
> > > >  - It can be overridden with `let hasSideEffects=1/0` on the instruction.
> > > > 
> > > > If you are looking to remove the side effects flags, and the instruction doesn't have a pattern, adding hasSideEffects=0 is usually a good way to go. Removing the side effects can help improve scheduling freedom and other code motion in the backend, so is usually good to do so long as the instruction doesn't do anything odd.
> > > Thansk @dmgreen  very much.
> > >    - As the above changes don't have some  overridden with `let hasSideEffects=1/0` , so I think the 2nd guidelines works in this case.
> > >    - Is the `def : LD1RPat<nxv4i32, load,        LD1RW_IMM,   PTRUE_S, i32, am_indexed32_6b, uimm6s4>;` before the change a tablegen pattern, and it also should set the instruction to false?
> > Yeah - IIRC, I think the logic in Tablegen might only apply when the pattern produces a single instruction. So the pattern with `(load (ptrue 31), ..` would not set `hasSideEffects=0` in the same way as the new `(load $pg, ..` pattern.
> Does hasSideEffects default to false? I investigated this after D140680 and spotted `bit hasSideEffects = ?;` with none of the base SVE instructions classes that sit above it setting it.  I suspect there was some AArch64 upstream refactoring between our original downstream implementation and when it was upstreamed that we missed and so this property has been dandling ever since (either that or it's just always been wrong).
> 
> Eitherway, as agreed on D140680 I'll have a patch in the next few days that should resolve it.  For this patch you can just accept the new value as has been the case of other patches where this flip has occurred.
Oops. Please ignore the final comment, I hadn't spotted this was not a new patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139637