[PATCH] D114460: [fir] Add fir reduction builder

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Just some initial comments (haven't managed to look through everything yet).



================
Comment at: flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h:31
+// and std::complex are layout compatible, but not compatible in all ABI call
+// interface (e.g. X86 32 bits). _Complex is not standard C++, so do not use
+// it here.
----------------



================
Comment at: flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h:62
+template <>
+constexpr TypeBuilderFunc getModel<short int>() {
+  return [](mlir::MLIRContext *context) -> mlir::Type {
----------------
Can we get rid of all of this duplication by using std::is_integral etc?


================
Comment at: flang/include/flang/Optimizer/Builder/Runtime/Reduction.h:21
+
+/// Generate call to all runtime routine.
+/// This calls the descriptor based runtime call implementation of the all
----------------
Should we use backticks or something around function names? It reads really strange otherwise.
Also, isn't this actually AllDim?


================
Comment at: flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h:57
+
+  // Commnly used type
+  mlir::Type i8Ty;
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114460