This is an archive of the discontinued LLVM Phabricator instance.

[flang][NFC] move constant lowering into its own unit
ClosedPublic

Authored by jeanPerier on Oct 28 2022, 7:57 AM.

Details

Summary

This patch moves intrinsic evaluate::Constant<T> lowering into its own
unit outside of ScalarExpr and genarr lowering so that it can
be used by the new lowering without any changes.

DerivedType lowering cannot be shared at that stage because it is too
correlated with the current lowering (requires structure constructor
and designator lowering).

The code had to be refactored quite a bit so that it could be carved
out, but the only "functional" change is that the length of character
arrays lowered by genarr is now index instead of i64 (see test change).
One non-functional benefit of the change is that toEvExpr is not
needed anymore and some compile time copies of big constant arrays
that it was causing are removed (see old calls in previous genarr code),
although I am not sure any compile time speed-ups are visible here.

Diff Detail

Event Timeline

jeanPerier created this revision.Oct 28 2022, 7:57 AM
Herald added a project: Restricted Project. · View Herald Transcript
jeanPerier requested review of this revision.Oct 28 2022, 7:57 AM
PeteSteinfeld accepted this revision.Oct 28 2022, 11:44 AM

Aside from a few nits and questions, all builds and tests correctly and looks good.

flang/include/flang/Lower/ConvertConstant.h
37

Should read "will contain".

flang/lib/Lower/ConvertConstant.cpp
265

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.

303

What does hashconsing mean?

346

Is it really possible for the size of the shape of the array constant to be zero?

358

Is it possible for the value of rangeSize to exceed the capacity of SmallVector?

399

Perhaps you mean "Create ... from" rather than "Create ... to".

412

"speeds" rather than "speed".

433

Perhaps you mean "Create ... from" rather than "Create ... to".

This revision is now accepted and ready to land.Oct 28 2022, 11:44 AM
jeanPerier marked 3 inline comments as done.

Fix typos in comments and remove complex case from convertToAttribute
(I was mislead into thinking complex could be converted to attribute
that way, but this code was actually unreachable and does not work
as pointed out in the DenseGlobalBuilder comment that I had moved
and not read well enough). Better to remove it IMHO.

Thanks for the review @PeteSteinfeld

flang/lib/Lower/ConvertConstant.cpp
265

I think it is needed for builds without asserts enabled because the 'len' is only used in an assert in the constepxr "then" case.

303

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

346

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

358

No because genInlinedArrayLit is only called from genArrayLit that is protected by the TODO you added for this.

399

I meant "Convert ... into" instead of "Create", thanks.

This revision was automatically updated to reflect the committed changes.