[flang-commits] [PATCH] D144102: [flang][hlfir] Array constructor lowering [part 1/4]

[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 some nits and questions, all builds and tests correctly and looks good.

I didn't understand Valentin's review.  He says "LGTM", but it looks like he didn't approve.  You might want to double check with him.



================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:48
+//   the array constructor, potentially reshaped, to an array variable.
+//
+//   The array constructor lowering looks like:
----------------
What about the case where there's an ac-implied-do-control, and the expression invokes an intrinsic that yields a constant?  Also, are you handling array constructors for arrays of derived types?


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:69-70
+namespace {
+/// Class that implements the "inlined temp strategy" to lower array
+/// constructor. It must be further provided a CounterType class to specify how
+/// the current ac-value insertion position is tracked.
----------------
This should either be "lower an array constructor" or "lower array constructors"


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:147
+/// The SSA value being tracked by the counter (hence, this
+/// cannot count through loops since the SSA value in the loop become
+/// inaccessible after the loop).
----------------
"become" should be "becomes"


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:151
+/// compile time constant bounds (even if huge). So this minimalistic
+/// counter greatly reduce the generated IR for simple but big array
+/// constructors [(i,i=1,constant-expr)] that are expected to be quite
----------------
"reduce" should be "reduces"


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:297
+/// Does the array constructor have length parameters that
+/// LengthAndTypeCollector::collect could not lower because this require
+/// lowering an ac-value and must be delayed?
----------------
"require" should be "requires"


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:310
+/// evaluate::ArrayConstructor before they are lowered. It does not generate any
+/// IR. The result of this analysis pass are used to select the lowering
+/// strategy.
----------------
"are used" should be "is used"


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:429
+  }
+  // Convert the array constructor type and try gather its length parameter
+  // values, if any.
----------------
"try gather" should be "try to gather"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144102