[PATCH] D118978: [flang] Basic local variable lowering

[Kiran Chandramohan via Phabricator via llvm-commits]

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

LGTM. Have suggested a few Nits.



================
Comment at: flang/include/flang/Lower/ConvertVariable.h:29
+/// The AbstractConverter builder must be set.
+/// The AbstractConverter own symbol mapping is not used during the
+/// instantiation and can be different form \p symMap.
----------------
Nit: AbstractConverter's


================
Comment at: flang/include/flang/Lower/ConvertVariable.h:30
+/// The AbstractConverter own symbol mapping is not used during the
+/// instantiation and can be different form \p symMap.
+void instantiateVariable(AbstractConverter &, const pft::Variable &var,
----------------
Nit: from


================
Comment at: flang/lib/Lower/Bridge.cpp:218
+      const Fortran::semantics::Symbol &sym = var.getSymbol();
+      if (!sym.IsFuncResult() || !funit.primaryResult)
+        instantiateVar(var);
----------------
Nit: what is a primaryResult?


================
Comment at: flang/lib/Lower/ConvertExpr.cpp:47
+  return addr.match(
+      [](const fir::CharBoxValue &box) -> fir::ExtendedValue { return box; },
+      [&](const fir::UnboxedValue &v) -> fir::ExtendedValue {
----------------
Nit: Can this be skipped/TODOed for now? Also not clear to me why there is no load here.


================
Comment at: flang/lib/Lower/ConvertExpr.cpp:49-51
+        if (fir::unwrapRefType(fir::getBase(v).getType())
+                .isa<fir::RecordType>())
+          return v;
----------------
Nit: Can this be skipped/TODOed for now? Also not clear to me why there is no load here.


================
Comment at: flang/lib/Lower/ConvertExpr.cpp:52
+          return v;
+        return builder.create<fir::LoadOp>(loc, fir::getBase(v));
+      },
----------------
Assuming this load is the one we require for the STOP test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118978