[PATCH] D123638: [SLP][AArch64] Implement lookahead operand reordering score of splat loads for AArch64

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2616
+      if (IsLoad) {
+        assert(isLegalBroadcastLoad(Tp->getElementType(),
+                                    LT.second.getVectorElementCount()) &&
----------------
dmgreen wrote:
> vporpo wrote:
> > dmgreen wrote:
> > > vporpo wrote:
> > > > dmgreen wrote:
> > > > > This assert looks like it will fire a lot of the time. Should we use `IsLoad && isLegalBroadcastLoad(...)`?
> > > > > 
> > > > > That might make this testable from the cost model too, Even if it's slightly unorthodox to use a vector load for such cases.
> > > > It is already guarded by `if (IsLoad)` so I guess `IsLoad && ` is not needed.
> > > > 
> > > > > That might make this testable from the cost model too, Even if it's slightly unorthodox to use a vector load for such cases.
> > > > I am not sure I understand. Is this about the assertion?
> > > I meant remove the assert and turn it into a condition. This function might be checking Types which we do not consider to be legal. https://godbolt.org/z/dv56WMaP7 has an example (apparently :) ).
> > > 
> > > We could presumably have a test in llvm/test/Analysis/CostModel/AArch64 that tests `load <vector>, splat-shuffle`, and it will trigger this and be treated as free?
> > Oops, yes this is totally broken, sorry about that. I had just copied this part of the code from X86 TTI where we have a cost table that checks the types but I skipped the table. This assertion is meant to check that the table and `isLegalBroadcastLoad` are in sync. 
> > To fix this I can either add a cost table like this:
> > ```
> >   static const CostTblEntry NeonBroadcastLoadTbl[] = { 
> >      {TTI::SK_Broadcast, MVT::v8i8, 0 },
> >      {TTI::SK_Broadcast, MVT::v16i8, 0 },
> >      {TTI::SK_Broadcast, MVT::v4i16, 0 },
> >      {TTI::SK_Broadcast, MVT::v8i16, 0 },
> >      {TTI::SK_Broadcast, MVT::v2i32, 0 },
> >      {TTI::SK_Broadcast, MVT::v4i32, 0 },
> >      {TTI::SK_Broadcast, MVT::v2f64, 0},
> >      {TTI::SK_Broadcast, MVT::v4f32, 0 },
> >   }; 
> > ```
> > using a similar logic as in X86TargetTransformInfo.cpp:1558, or rely on an `if (isLegalBroadcastLoad())`. 
> > I think adding the table makes it a bit more explicit, and I would prefer it. What do you think? 
> > 
> > Btw would you happen to know if a `v2f32` broadcast is handled by `ld1r` in neon? I think this is the only 64-bit entry missing from the table.
> v2f32 should work OK - there is nothing very different between a v2f32 and a v2i32 load. There are also fp16 types and possibly bf16 types (but they might not work at the moment).
> 
> My slight preference would probably be towards re-using isLegalBroadcastLoad because they are then insync by design and we don't need to repeat the logic. Up to you though.
> We could presumably have a test in llvm/test/Analysis/CostModel/AArch64 that tests load <vector>, splat-shuffle, and it will trigger this and be treated as free?
This won't work with the current code. TTI's `getUserCost()` won't pass shuffle's  operands to `TargetTTI->getShuffleCost()`. This would have to be part of the refactoring patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123638