[PATCH] D153808: [CodeGen] Add support for integers using SVE2 in ComplexDeinterleaving passDepends on D153355

Tue Jun 27 06:22:10 PDT 2023

igor.kirillov added inline comments.

================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:1366
               ComplexDeinterleavingOperation::Symmetric, nullptr, nullptr);
-          TmpNode->Opcode = Instruction::FAdd;
-          TmpNode->Flags = Flags;
+          if (Flags) {
+            TmpNode->Opcode = Instruction::FAdd;
----------------
mgabka wrote:
> in my opinion it would make more sense to distinguishing between fp and int operations based on the original instructions, not based on extra flags we want to propagate.
> It would make the code more logical, i.e if fp propagate flag and set to fadd.
> Also I recall that this pass can match things produced by -O3, are the flags set in that case as well?
> 
>  how difficult it would be to change the logic to the one I propose?
**FFastMathOperation** always has **FFastFlags**; any Integer operation doesn't have them. So, I have to choose between having two variables (bool HasFastFlags, FFastFlags Flags) or having only one variable that incorporates this information (std::optional<FFastFlags>). I suggest using the second approach and reducing the number of entities. 

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:25666
       return B.CreateIntrinsic(
-          Intrinsic::aarch64_sve_fcmla, Ty,
+          IsInt ? Intrinsic::aarch64_sve_cmla_x : Intrinsic::aarch64_sve_fcmla,
+          Ty,
----------------
mgabka wrote:
> the IsInt check he is redundant here as it is already covered above
Yes, that's a rudiment of my first attempt to handle the case. But integer cmla doesn't accept a mask as an argument.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153808