[PATCH] D140069: [DAGCombiner] Scalarize vectorized loads that are splatted

Tue Dec 20 08:22:35 PST 2022

luke added inline comments.

================
Comment at: llvm/test/CodeGen/AArch64/arm64-vmul.ll:1102-1106
+; CHECK-NEXT:    add x8, x1, #2
+; CHECK-NEXT:    ldr q1, [x0]
+; CHECK-NEXT:    ld1r.8h { v0 }, [x8]
+; CHECK-NEXT:    mul.8h v0, v1, v0
 ; CHECK-NEXT:    ret
----------------
dmgreen wrote:
> luke wrote:
> > luke wrote:
> > > Regression: Can the `add` be folded in as an immediate offset to `ld1r.8h { v0 }, [x8]`?
> > > Same applies for the cases below
> > No, since the offset would actually increment the register operand. 
> > Should we instead check if the target is able to perform an indexed load, and bail otherwise when the offset != 0?
> > There's a target lowering hook called `isIndexingLegal` that seems like it could be used to check this, but no targets currently implement it, and it was added for GlobalISel: https://reviews.llvm.org/D66287
> > @t.p.northover would you have any thoughts on this?
> Hello. This can actually be worse, but I don't think that's an issue with this patch. We tend to prefer ld1r over the dup in the mul instruction, but I believe the opposite can be quicker. That is a general issue though, this patch is just exposing it in a few extra places.
> 
> None of these tests need to load data, and I think it would be better to remove that part. If they just use vectors parameters directly then they will be less susceptible to optimizations on the load altering what the test is intended to check. I can put a patch together to clean this up, and if you rebase on top most of these changes should hopefully disappear.
Hello, thanks a million for that patch. Will rebase.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140069