This is an archive of the discontinued LLVM Phabricator instance.

Support for PowerPC vector type - derived type based representation
ClosedPublic

Authored by tislam on May 18 2023, 8:58 AM.

Details

Summary

This revision is a followup to Support for PowerPC vector type. The current solution internally represents PowerPC vector types as derived-types. A new builtin module, named __fortran_ppc_types has been added to declare the corresponding derived-types.

The idea of using derived-types was suggested by Jean Perier and Peter Klausler.

Contributors:
Coauthor: Daniel Chen (IBM)
Co-designers: Daniel Chen (IBM), Kelvin Li (IBM)

For convenience, the summary from the original review is added below.

This RFC is for adding the PowerPC vector type support as a language extension.

It has the following syntax:

VECTOR(element-type-spec)

where

element-type-spec

is:

integer-type-spec

or

real-type-spec

or

unsigned-type-spec

A finite set of functionalities are implemented in order to support the PowerPC vector type:

  1. declare VECTOR type objects.
  2. declare VECTOR type function result.
  3. declare VECTOR type dummy arguments.
  4. intrinsic assignment between VECTOR type objects. (e.g. v1 = v2)
  5. reference VECTOR functions.

This patch only supports the functionalities mentioned at the above. Others, such as intrinsic operators are not extended to support vectors as all operations on vector types (such as addition, subtraction, construction, ... etc) are done via calls to vector intrinsic functions. The support of constant expressions, derived component, POINTER or ALLOCATABLE attribute, I/O, and etc is not included in this patch.

The purpose of this RFC is to get comments from the community on our implementation approach. We plan to add the LIT tests in this patch as a revision soon. We also plan to add semantic checking in a separate patch in the near future.

Diff Detail

Event Timeline

tislam created this revision.May 18 2023, 8:58 AM
tislam requested review of this revision.May 18 2023, 8:58 AM

This is an excellent patch, and I have no comments apart from some easily-fixed minor concerns.

flang/include/flang/Common/Fortran-features.h
39

Can you please use a more specific name, like PPCVector? Thanks

flang/include/flang/Parser/parse-tree.h
715–717

You don't need the explicit constructors here. UNION_CLASS_BOILERPLATE supplies a template constructor for you.

flang/include/flang/Semantics/type.h
252

Consider using the more modern enum class feature here instead of a plain enum. It enables the C++ build compiler to check for missing cases in switch statements.

flang/lib/Evaluate/type.cpp
168

common::die() or DIE() is better for crashes that aren't really about unreachable code.

flang/lib/Parser/Fortran-parsers.cpp
175

Please make this new alternative the last one, not the first, so that compilations of code without PPC vector types are not slowed down by this change.

228

The _sptok suffixes here are probably not what you want.

flang/lib/Semantics/resolve-names-utils.cpp
618

Is the extra test needed here? What happens if it's not present?

flang/lib/Semantics/resolve-names.cpp
1094

Trailing underscore on the private component name, please.

4703

Will programs using non-constant kind expressions cause a crash here? You might want to change the result type to std::optional<int> and be more error-recoverable.

tschuett added inline comments.
flang/lib/Lower/ConvertType.cpp
364

error message or die. The place is reachable.

flang/lib/Semantics/resolve-names.cpp
4703

error message or die. optional might be confusing.

tislam updated this revision to Diff 523513.May 18 2023, 1:07 PM

Addressed the following review comments:

  • flang/include/flang/Common/Fortran-features.h: Changed enum name from Vector to PPCVector.
  • flang/include/flang/Parser/parse-tree.h: Removed explicit constructors for VectorElementType.
  • flang/include/flang/Semantics/type.h: Changed enum to ENUM_CLASS for Category inside DerivedTypeSpec.
  • Replaced llvm_unreachable with common::die.
  • Updated parsing rules.
  • flang/lib/Semantics/resolve-names.cpp: Renamed isVectorType to isVectorType_.
  • Added flang/module/__fortran_ppc_types.f90. This file was missing in the first version.
tislam marked 9 inline comments as done.May 18 2023, 1:12 PM
tislam added inline comments.
flang/lib/Semantics/resolve-names-utils.cpp
618

Without the test, Flang does not allow entities of vector type in equivalence.

flang/lib/Semantics/resolve-names.cpp
4703

Using common::die here.

tislam edited the summary of this revision. (Show Details)May 18 2023, 5:29 PM
tschuett added inline comments.May 18 2023, 9:21 PM
flang/include/flang/Semantics/type.h
252

enum class Category {DerivedType, IntrinsicVector, PairVector, QuadVector};

Is enum class insufficient or because of style?

tschuett added a comment.EditedMay 18 2023, 10:54 PM

Do you want to add an unparse test?
There are no tests?

Is the usage limited to PowerPC?

tislam updated this revision to Diff 523944.May 19 2023, 2:51 PM

Changes in this revision:

  • flang/include/flang/Semantics/type.h: Changed ENUM_CLASS to enum class for Category in DerivedTypeSpec.
  • Added a new test (flang/test/Semantics/ppc-vector-types.f90) to verify unparsing.
  • Added a new test (flang/test/Lower/ppc-vector-types.f90) to verify lowering.

Do you want to add an unparse test?
There are no tests?

Thanks @tschuett.
I have added two tests to verify the unparsing and lowering of vector types.

Is the usage limited to PowerPC?

That is correct. The current patch is targetted for PowerPC.

tislam marked an inline comment as done.May 19 2023, 2:56 PM
tislam added inline comments.
flang/include/flang/Semantics/type.h
252

I have changed this to enum class.

tislam marked an inline comment as done.May 19 2023, 2:57 PM
klausler accepted this revision.May 19 2023, 5:16 PM
This revision is now accepted and ready to land.May 19 2023, 5:16 PM

Where do you check that it is only available on PowerPC?

tschuett added inline comments.May 20 2023, 12:45 AM
flang/lib/Semantics/semantics.cpp
511–512

I found it!

tschuett accepted this revision.May 20 2023, 12:45 AM
This revision was automatically updated to reflect the committed changes.