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

[mgabka via Phabricator via llvm-commits]

mgabka added inline comments.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:956
                                                  Instruction *Imag) {
+  auto IsOperationsupported = [](unsigned Opcode) -> bool {
+    return Opcode == Instruction::FAdd || Opcode == Instruction::FSub ||
----------------
probably should start with capital S


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:970
 
-  FastMathFlags Flags = Real->getFastMathFlags();
-  if (!Flags.allowReassoc()) {
-    LLVM_DEBUG(
-        dbgs() << "the 'Reassoc' attribute is missing in the FastMath flags\n");
-    return nullptr;
+  if (Flags) {
+    if (Real->getFastMathFlags() != Imag->getFastMathFlags()) {
----------------
wouldn't be more clear to guard it by if(isa<FPMathOperator>(Real)) ?


on the same from do we need to have a check somewhere that Real and Imag are of the same type? i.e both fp or integer?


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:1027-1029
+        if (isNeg(I)) {
+          Worklist.emplace_back(getNegOperand(I), !IsPositive);
+        } else {
----------------
looking at the diff this looks like a new functionality, maybe worth to add it as a separate patch?


================
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;
----------------
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?


================
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,
----------------
the IsInt check he is redundant here as it is already covered above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153808