[PATCH] D114125: [Flang] Add a factory class for creating Complex Ops

[Diana Picus via Phabricator via llvm-commits]

rovka accepted this revision.
rovka added a comment.

LGTM with nits.



================
Comment at: flang/include/flang/Optimizer/Builder/Complex.h:32
+  /// Get the Complex Type. Determine the type. Do not create MLIR operations.
+  mlir::Type getComplexPartType(mlir::Value cplx);
+  mlir::Type getComplexPartType(mlir::Type complexType);
----------------
Nit: Can these methods be const?


================
Comment at: flang/include/flang/Optimizer/Builder/Complex.h:43
+
+  mlir::Value extractComplexPart(mlir::Value cplx, bool isImagPart) {
+    return isImagPart ? extract<Part::Imag>(cplx) : extract<Part::Real>(cplx);
----------------
Nit: Why not pass the Part directly, instead of a bool?


================
Comment at: flang/lib/Optimizer/Builder/Complex.cpp:27
+  auto complexTy = fir::ComplexType::get(builder.getContext(), kind);
+  mlir::Value und = builder.create<fir::UndefOp>(loc, complexTy);
+  return insert<Part::Imag>(insert<Part::Real>(und, real), imag);
----------------



================
Comment at: flang/unittests/Optimizer/Builder/ComplexTest.cpp:88
+  EXPECT_TRUE(fir::isa_real(helper->getComplexPartType(cVal4)));
+}
+
----------------
Can you add another test case to check the values that are inserted and extracted? (I.e. not just the types).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114125