[PATCH] D101782: [AArch64][GlobalISel] Add post-legalizer lowering for NEON vector fcmps

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64Combine.td:176
+    [{ return lowerVectorFCMP(*${root}, MRI, B); }]),
+  (apply [{ return true; }])>;
+
----------------
apply functions don't need to return a value.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerLowering.cpp:858
+              bool NoNans, MachineRegisterInfo &MRI) {
+  LLT DstTy = MRI.getType(LHS);
+  std::function<Register(MachineIRBuilder &)> Func;
----------------
Should assert that the type is a vector.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerLowering.cpp:915
+    };
+    return Func;
+  }
----------------
Why not just return the lambda directly instead of via Func?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerLowering.cpp:947
+  MIB.setInstrAndDebugLoc(MI);
+  auto Cmp = getVectorFCMP(CC, LHS, RHS, IsZero, NoNans, MRI);
+  if (!Cmp)
----------------
When can this be None? Is there missing support that needs to be added later, or should this be more of an assert?


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

https://reviews.llvm.org/D101782