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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan 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:
> rovka wrote:
> > Nit: Can these methods be const?
> I meant the methods themselves, not the return type, i.e.
> mlir::Type getComplexPartType(...) const;
> 
my bad. Corrected.


================
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);
----------------
rovka wrote:
> 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.
I am leaving it as is. I don't have a strong opinion. It kind of matches the way some information is there in the Semantics phase. See example below.


```
  template <int KIND>
  CC genarr(const Fortran::evaluate::ComplexComponent<KIND> &x) {
    auto loc = getLoc();
    auto lambda = genarr(x.left());
    auto isImagPart = x.isImaginaryPart;
    return [=](IterSpace iters) -> ExtValue {
      auto lhs = fir::getBase(lambda(iters));
      return fir::factory::Complex{builder, loc}.extractComplexPart(lhs,
                                                                    isImagPart);
    };  
  }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114125