[PATCH] D146218: [AArch64][CodeGen] Lower (de)interleave2 intrinsics to ld2/st2

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14765
+  if (EC.isScalable()) {
+    UseScalable = true;
+    return isPowerOf2_32(MinElts) && (MinElts * ElSize) % 128 == 0;
----------------
mgabka wrote:
> huntergr wrote:
> > mgabka wrote:
> > > is it correct to set it here to true or only if the condition below is true?
> > > 
> > > from what I can see, before UseScalable was only set to true if this function was returning true
> > It's fine -- if the type is not a legal shuffle type, no transformation will be performed.
> if callers check correctly for the return value of this function.
> However, the UseScalable is already set to false, even if the function returns false, my point is that this function should modify state of UseScalable only if it returns true.
> But that is a just my personal preference I guess.
The first part is the function's design (i.e. UseScalable only contains meaning data when the function returns true).  That said, this function is not great as it tries to do two different jobs.  Certainly worth a redesign if there's chance to refactor the current way fixed length vectors are implemented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146218