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

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
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);
----------------
rovka wrote:
> Nit: Can these methods be const?
I meant the methods themselves, not the return type, i.e.
mlir::Type getComplexPartType(...) 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);
----------------
kiranchandramohan wrote:
> rovka wrote:
> > Nit: Why not pass the Part directly, instead of a bool?
> I think this is a convenience function for external users. i.e they don't have to know about Part.
I guess this is a matter of taste, but using Part instead of bool would make calls inherently clear, without people having to spell out /* isImagPart=*/ before the argument. Plus, Part is already public. 

That's just my 2c, feel free to leave it as is.


================
Comment at: flang/unittests/Optimizer/Builder/ComplexTest.cpp:88
+  EXPECT_TRUE(fir::isa_real(helper->getComplexPartType(cVal4)));
+}
+
----------------
kiranchandramohan wrote:
> rovka wrote:
> > Can you add another test case to check the values that are inserted and extracted? (I.e. not just the types).
> I think since there are insertsOps and ExtractOps, it will be difficult to do that in the unittest. If I am missing a point, please let me know.
> 
> IR testing will come along with lowering later.
Fair enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114125