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

Mon Jun 26 07:20:18 PDT 2023

huntergr 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;
----------------
paulwalker-arm wrote:
> 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.
I agree with Paul that it conflates two jobs, but I did just think of a nicer way to do it -- return either a pair of bools or an optional<bool> instead of passing in a reference to the variable. I can make that a fixup commit if you'd like.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146218