[flang-commits] [PATCH] D136955: [flang][NFC] move constant lowering into its own unit

[Pete Steinfeld via Phabricator via flang-commits]

PeteSteinfeld accepted this revision.
PeteSteinfeld added a comment.
This revision is now accepted and ready to land.

Aside from a few nits and questions, all builds and tests correctly and looks good.



================
Comment at: flang/include/flang/Lower/ConvertConstant.h:36
+  /// constants will be lowered into read only memory fir.global, and the
+  /// resulting fir::ExtendedValue will contains the address of the fir.global.
+  /// This option should not be set if the constant is being lowered while the
----------------
Should read "will contain".


================
Comment at: flang/lib/Lower/ConvertConstant.cpp:264
+                      Fortran::common::TypeCategory::Character, KIND>> &value,
+                  [[maybe_unused]] int64_t len) {
+  if constexpr (KIND == 1) {
----------------
Is this `[[maybe_unused]]` modifier needed?  It looks like `len` is used on both the `then` and `else` parts of the `if constexpr` statement.  I was able to build without it using g++ 9.3.0, if that's any indication.


================
Comment at: flang/lib/Lower/ConvertConstant.cpp:302
+  // Otherwise, the string is in a plain old expression so "outline" the value
+  // in read only data by hashconsing it to a constant literal object.
+
----------------
What does `hashconsing` mean?


================
Comment at: flang/lib/Lower/ConvertConstant.cpp:345
+  mlir::Value array = builder.create<fir::UndefOp>(loc, arrayTy);
+  if (Fortran::evaluate::GetSize(con.shape()) == 0)
+    return array;
----------------
Is it really possible for the size of the shape of the array constant to be zero?


================
Comment at: flang/lib/Lower/ConvertConstant.cpp:357
+    llvm::SmallVector<mlir::Attribute> rangeStartIdx;
+    uint64_t rangeSize = 0;
+    mlir::Type eleTy = arrayTy.cast<fir::SequenceType>().getEleTy();
----------------
Is it possible for the value of `rangeSize` to exceed the capacity of `SmallVector`?


================
Comment at: flang/lib/Lower/ConvertConstant.cpp:398
+
+/// Create an evaluate::Constant<T> array to a fir.ref<fir.array<>> value
+/// that points to the storage of a fir.global in read only memory and is
----------------
Perhaps you mean "Create ... from" rather than "Create ... to".


================
Comment at: flang/lib/Lower/ConvertConstant.cpp:411
+    // Using a dense attribute for the initial value instead of creating an
+    // intialization body speed up MLIR/LLVM compilation, but this is not always
+    // possible.
----------------
"speeds" rather than "speed".


================
Comment at: flang/lib/Lower/ConvertConstant.cpp:432
+
+/// Create an evaluate::Constant<T> array to an fir::ExtendedValue.
+template <Fortran::common::TypeCategory TC, int KIND>
----------------
Perhaps you mean "Create ... from" rather than "Create ... to".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136955