[PATCH] D112076: [AArch64][SVE][InstCombine] Combine contiguous gather/scatter to load/store

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:880
+  Value *IndexBase;
+  if (match(Index, m_Intrinsic<Intrinsic::aarch64_sve_index>(
+                       m_Value(IndexBase), m_SpecificInt(1)))) {
----------------
peterwaller-arm wrote:
> david-arm wrote:
> > nit: Is it worth reversing the logic to remove indentation and avoid unnecessary creation of variables above? For example, something like this:
> > 
> >   Value *Index = II.getOperand(2);
> >   Value *IndexBase;
> >   if (!match(Index, m_Intrinsic<Intrinsic::aarch64_sve_index>(
> >       m_Value(IndexBase), m_SpecificInt(1))))
> >     return None;
> > 
> >   Value *Mask = II.getOperand(0);
> >   Value *BasePtr = II.getOperand(1);
> >   ...
> > 
> > 
> As with many other combines, I've structured it in this way to enable further combines to be added without having to touch my logic.
> 
> (I'm also a fan of early return in principle *tips hat*, but I think it might not be best for this case).
> 
I'm also making a loose assumption here that those variable assignments will optimize to be free or very cheap in the case where they are not used. Additionally, that those variable assignments are likely to find use in hypothetical future combines. These points are all debatable, I think!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112076