This is an archive of the discontinued LLVM Phabricator instance.

[flang] Save AllocateObject and PointerObject analyzed expression
ClosedPublic

Authored by jeanPerier on Mar 9 2021, 6:16 AM.

Details

Summary

parser::AllocateObject and parser::PointerObject can be represented
as typed expressions once analyzed. This simplifies the work for parse-tree
consumers that work with typed expressions to deal with allocatable and
pointer objects such as lowering.

This change also makes it easier to add typedExpr in the future by
automatically handling nodes that have this member when possible.

Changes:

  • Add a mutable TypedExpr typedExpr field to parser::PointerObject and parser::AllocateObject.
  • Add a parser::HasTypedExpr<T> helper to better share code relating to typedExpr in the parse tree.
  • Add hooks in semantics::ExprChecker for AllocateObject and PointerObject nodes, and use ExprOrVariable on it to analyze and set the tyedExpr field during expression analysis. This required adding overloads for AssumedTypeDummy.
  • Update check-nullify.cpp and check-deallocate.cpp to not re-analyze the StructureComponent but to use the typedExpr field instead.
  • Update dump/unparse to use HasTypedExpr and use the typedExpr when there is one.

Diff Detail

Event Timeline

jeanPerier created this revision.Mar 9 2021, 6:16 AM
jeanPerier requested review of this revision.Mar 9 2021, 6:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2021, 6:16 AM
PeteSteinfeld accepted this revision.Mar 9 2021, 8:06 AM

These changes build,install, and look good.

Note that (unrelated to these changes), my latest version of clang-format shows that there's an extra blank line at line 3735 in parse-tree.h.

Also, when I was working on a bug in check-nullify.cpp, I was initially baffled by the fact that the analysis of the PointerObject's wasn't done in expression analysis rather than in semantic checking. Do you know what the trade-offs are for analyzing expressions for ALLOCATE, DEALLOCATE, and NULLIFY expressions in semantic analysis rather than expression analysis?

This revision is now accepted and ready to land.Mar 9 2021, 8:06 AM
tskeith accepted this revision.Mar 9 2021, 9:45 AM
tskeith added inline comments.
flang/lib/Semantics/check-deallocate.cpp
46

This case could be simplified slightly:

MaybeExpr checked{analyzer.Analyze(structureComponent)};
if (checked && ...
}
return checked;

This reduces the indentation and make the explicit return type unnecessary.

Also, when I was working on a bug in check-nullify.cpp, I was initially baffled by the fact that the analysis of the PointerObject's wasn't done in expression analysis rather than in semantic checking. Do you know what the trade-offs are for analyzing expressions for ALLOCATE, DEALLOCATE, and NULLIFY expressions in semantic analysis rather than expression analysis?

You are raising an interesting point, I do not know. It might make sense to move this in semantics::ExprChecker. Let me have a look.

klausler requested changes to this revision.Mar 9 2021, 10:18 AM

If you grep the sources for "typedExpr" you'll find some other places that should be changed (unparse.cpp, Semantics/tools.cpp, dump-parse-tree.h). And you should think about moving this code into Semantics/expression.cpp to be near the other stuff that analyzes and saves typed expressions.

This revision now requires changes to proceed.Mar 9 2021, 10:18 AM

Move the typedExpr setting in expression analysis. Update dump and unparse to use
the typed expression. Add a HasTypeExpr<T> helper to better share the logic when that
is possible and easy.

Update check-nullify.cpp/check-deallocate.cpp to use the typedExpr instead of
re-analysizing the expression.

jeanPerier edited the summary of this revision. (Show Details)Mar 11 2021, 4:51 AM
jeanPerier marked an inline comment as done.Mar 15 2021, 8:02 AM
jeanPerier added inline comments.
flang/lib/Semantics/check-deallocate.cpp
46

This comment is not really applicable anymore after the patch update.

klausler accepted this revision.Mar 15 2021, 9:29 AM

Looks great; thanks!

This revision is now accepted and ready to land.Mar 15 2021, 9:29 AM
This revision was landed with ongoing or failed builds.Mar 16 2021, 2:30 AM
This revision was automatically updated to reflect the committed changes.
jeanPerier marked an inline comment as done.

Are these warning problems? Is there a reason these aren’t errors? Or have associated file names and line numbers?

% bbc /proj/ta/tests/nag_f95/chkio.f90
warning: TODO: extended value is missing type parameters
warning: TODO: extended value is missing type parameters
warning: TODO: extended value is missing type parameters
warning: TODO: extended value is missing type parameters

They are reminders that there are TODOs with PDTs. You can ignore them for the moment.

From: Steve Scalpone <sscalpone@nvidia.com>
Sent: Monday, April 5, 2021 11:32 PM
To: Eric Schweitz <eschweitz@nvidia.com>; Jean Perier <reviews+D98256+public+4c24a472db43530f@reviews.llvm.org>
Subject: Bbc "warning" without source position

Are these warning problems? Is there a reason these aren’t errors? Or have associated file names and line numbers?

% bbc /proj/ta/tests/nag_f95/chkio.f90
warning: TODO: extended value is missing type parameters
warning: TODO: extended value is missing type parameters
warning: TODO: extended value is missing type parameters
warning: TODO: extended value is missing type parameters