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

[Jean Perier via Phabricator via flang-commits]

jeanPerier added a comment.

Thanks for the review @PeteSteinfeld



================
Comment at: flang/lib/Lower/ConvertConstant.cpp:264
+                      Fortran::common::TypeCategory::Character, KIND>> &value,
+                  [[maybe_unused]] int64_t len) {
+  if constexpr (KIND == 1) {
----------------
PeteSteinfeld wrote:
> 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.
I think it is needed for builds without asserts enabled because the 'len' is only used in an assert in the constepxr "then" case. 


================
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.
+
----------------
PeteSteinfeld wrote:
> What does `hashconsing` mean?
I took this comment from the current code: https://github.com/llvm/llvm-project/blob/21b825738b7e4f45a42cd5fb1b8480996677fc9e/flang/lib/Lower/ConvertExpr.cpp#L1614.
It means making a name for the constant that is unique (enough) with a hash of the value.
I added a space ("hash consing") so that it matches the wikipedia definition: https://en.wikipedia.org/wiki/Hash_consing


================
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;
----------------
PeteSteinfeld wrote:
> Is it really possible for the size of the shape of the array constant to be zero?
Yes., that makes little sense, but everything that is not forbidden is possible. See for instance:

```
  integer, parameter :: x(0) = 42
  print *, size(x)
end
```

Val  added this when fixing couple issues with zero sized arrays: https://github.com/flang-compiler/f18-llvm-project/commit/539132e2ff5961e5daf43ebeed052afa8e31ae3


================
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();
----------------
PeteSteinfeld wrote:
> Is it possible for the value of `rangeSize` to exceed the capacity of `SmallVector`?
No because `genInlinedArrayLit` is only called from genArrayLit that is protected by the TODO you added for this.


================
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
----------------
PeteSteinfeld wrote:
> Perhaps you mean "Create ... from" rather than "Create ... to".
I meant "Convert ... into" instead of "Create", thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136955