[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)))) {
----------------
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).



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:897
+    MaskedLoad->takeName(&II);
+    return IC.replaceInstUsesWith(II, MaskedLoad);
+  }
----------------
david-arm wrote:
> Should we also call `II.eraseFromParent();` here too like the scatter case?
Nice nitpick, but my belief is that the IC.replaceInstUsesWith-related logic erases it in the load case because loads return a value. When these uses are removed the load becomes trivially dead and deleted. Whereas the stores do not have any uses, and therefore the erasure logic doesn't kick in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112076