[PATCH] D114900: [fir] Add fir character builder

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: flang/include/flang/Optimizer/Builder/Runtime/Character.h:49
+/// runtime entry function.
+void genAdjust(fir::FirOpBuilder &builder, mlir::Location loc,
+               mlir::Value resultBox, mlir::Value stringBox,
----------------
Nit: Is this actually used from outside this module, or can it be downgraded to a helper in the implementation file?


================
Comment at: flang/lib/Optimizer/Builder/Runtime/Character.cpp:105
+
+mlir::Value fir::runtime::genCharCompare(fir::FirOpBuilder &builder,
+                                         mlir::Location loc,
----------------
Please add a test for this (and check the Alloca and Store in it too).


================
Comment at: flang/lib/Optimizer/Builder/Runtime/Character.cpp:111
+  if (lhs.getBoxOf<fir::BoxValue>() || rhs.getBoxOf<fir::BoxValue>())
+    TODO(loc, "character compare from descriptors");
+  auto allocateIfNotInMemory = [&](mlir::Value base) -> mlir::Value {
----------------
I'm a bit confused, aren't descriptors the main thing we're supposed to handle here?
Also, why is this calling the other overload? I would expect it to generate calls to CharacterCompareScalar, not CharacterCompareScalarN.


================
Comment at: flang/lib/Optimizer/Builder/Runtime/Character.cpp:200
+  genCharacterSearch(func, builder, loc, resultBox, stringBox, setBox, backBox,
+                     kind);
+}
----------------
This can also take source arguments.


================
Comment at: flang/lib/Optimizer/Builder/Runtime/Character.cpp:243
+  genCharacterSearch(func, builder, loc, resultBox, stringBox, setBox, backBox,
+                     kind);
+}
----------------
This can also take source args.


================
Comment at: flang/lib/Optimizer/Builder/Runtime/Character.cpp:284
+
+  auto fTy = func.getType();
+  auto sourceFile = fir::factory::locationToFilename(builder, loc);
----------------
Nit: I think you can move the definition to the top of the file (i.e. merge it with the declaration).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114900