[PATCH] D118786: [flang] Add lowering for integer constant

[Kiran Chandramohan via Phabricator via llvm-commits]

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

Thanks, @clementval. Great job in factoring out the scalar integer expressions.

Could you add to the commit message that `genExprValue` is the top-level function that is used in this patch? Will be a good starting point for reviewing.

LGTM.



================
Comment at: flang/lib/Lower/ConvertExpr.cpp:251
+      TODO(getLoc(), "genval array constant");
+    auto opt = con.GetScalarValue();
+    assert(opt.has_value() && "constant has no value");
----------------
Nit: Can the auto be removed here? 
std::optional<Scalar<Type<TypeCategory::Character, KIND>>> ?


================
Comment at: flang/lib/Lower/ConvertExpr.cpp:308
+  /// Helper to package a Value and its properties into an ExtendedValue.
+  static ExtValue toExtendedValue(mlir::Location loc, mlir::Value base,
+                                  llvm::ArrayRef<mlir::Value> extents,
----------------
Nit: Is this used in this or the related patches? If not I think it might be better to skip in this one.


================
Comment at: flang/lib/Lower/ConvertType.cpp:499
+  }
+  llvm_unreachable("INTEGER type translation not implemented");
+}
----------------
Nit: Should this be "non-INTEGER type translation not implemented"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118786