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

[Valentin Clement via Phabricator via llvm-commits]

clementval marked 5 inline comments as done.
clementval added inline comments.


================
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 {
----------------
rovka wrote:
> 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.
Lowering is actually done is a different way at the moment so we don't need this. If lowering stays as of today then this could be turn into an assert but we might update it in the future so a TODO seems the right thing right now. 


================
Comment at: flang/lib/Optimizer/Builder/Runtime/Character.cpp:200
+  genCharacterSearch(func, builder, loc, resultBox, stringBox, setBox, backBox,
+                     kind);
+}
----------------
rovka wrote:
> This can also take source arguments.
Not sure In understand your comment. Source args are extracted from `loc` and added as arguments in `genCharacterSearch`


================
Comment at: flang/lib/Optimizer/Builder/Runtime/Character.cpp:243
+  genCharacterSearch(func, builder, loc, resultBox, stringBox, setBox, backBox,
+                     kind);
+}
----------------
rovka wrote:
> This can also take source args.
Yeah there are extracted from `loc` in `genCharacterSearch`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114900