[RFC]Add align pragma for derived type and fix shape array/character type #1360

JiaweiHawk · 2023-04-07T12:13:51Z

Add support for aligning the first address of some type with configurable alignment.

The overall idea for this patch set is as follows:

add flg.x[251] for saving pragma alignment value in do_sw()
add the stg_align field in STG_MEMBERS(dt) to save the pragma alignment value corresponding to the dtype
update the alignment value in alignment() and lower_put_datatype() in flang1
parse the pragma alignment value passed by flang1 through stb in flang2 in read_datatype()
update the alignment value in alignment() in flang2
each common block should be aligned to the largest alignment value of the type it contains, flang2 should update the expected alignment of a block in gen_acon_expr()
the offset of each object to relative common block should be aligned to relative dtype alignment in gen_acon_expr()

Finally, the following effects can be achieved:

!dir$ align 0x100
character(len=10)   :: v1

!dir$ align 0x200
type T4
    integer(kind=2)     :: f1
    integer(kind=4)     :: f2
end type T4

!dir$ align 0x400
integer, dimension (5,5) :: v2

I test locally through make check-flang, and also pass some custom tests and add them to the test folder in patch.
Please correct me if I am wrong, and I am looking forward to feedback for this patch set.

JiaweiHawk · 2023-04-13T13:33:42Z

Gentle Ping...

kiranchandramohan · 2023-04-13T14:11:12Z

Thanks @JiaweiHawk for this RFC and patch.

We would like to support directives in commonly used formats. Is this based on some other compiler's directives? The only one I could find was Intel's which has the syntax !DIR$ ATTRIBUTES ALIGN : 64 :: R_alloc
https://www.intel.com/content/www/us/en/docs/fortran-compiler/developer-guide-reference/2023-0/attributes-align.html

Also think having a hex number is probably not good for readability.

JiaweiHawk · 2023-04-14T09:00:39Z

Thank @kiranchandramohan for reviewing.

We would like to support directives in commonly used formats. Is this based on some other compiler's directives? The only one I could find was Intel's which has the syntax !DIR$ ATTRIBUTES ALIGN : 64 :: R_alloc https://www.intel.com/content/www/us/en/docs/fortran-compiler/developer-guide-reference/2023-0/attributes-align.html

I chose this format only because I found the keyword "SW_ALIGN" in the source code, so I reused that part of the code, which resulted in this format.
It is okay for me to use syntax !DIR$ ATTRIBUTES ALIGN : alignment :: object, I will refactor my patch to support this format.

Also think having a hex number is probably not good for readability.

Here I did not specify a particular format for the numbers, I just use gtok() and itok to get its value, so both hexadecimal and decimal formats should work. However, I prefer hex number, because the alignment value should be a power of 2, which is clearer when represented in hexadecimal format.

By the way, there seems to be some errors in the test cases added in this patch, I will try to fix them later.

test/directives/align_character_type.f90

test/directives/align_array_type.f90

pawosm-arm · 2023-04-21T14:57:19Z

Thanks @JiaweiHawk for this RFC and patch.

We would like to support directives in commonly used formats. Is this based on some other compiler's directives? The only one I could find was Intel's which has the syntax !DIR$ ATTRIBUTES ALIGN : 64 :: R_alloc https://www.intel.com/content/www/us/en/docs/fortran-compiler/developer-guide-reference/2023-0/attributes-align.html

Also think having a hex number is probably not good for readability.

IMO if the cost of sticking to Intel's syntax is too high and requires major refactor, then we'd rather use the simpler syntax proposed by the author (as it is simpler and straightforward). I do agree though about use of hex, it's uncommon in the Fortran world.

JiaweiHawk · 2023-04-25T07:25:39Z

Thanks @JiaweiHawk for this RFC and patch.
We would like to support directives in commonly used formats. Is this based on some other compiler's directives? The only one I could find was Intel's which has the syntax !DIR$ ATTRIBUTES ALIGN : 64 :: R_alloc https://www.intel.com/content/www/us/en/docs/fortran-compiler/developer-guide-reference/2023-0/attributes-align.html
Also think having a hex number is probably not good for readability.

IMO if the cost of sticking to Intel's syntax is too high and requires major refactor, then we'd rather use the simpler syntax proposed by the author (as it is simpler and straightforward). I do agree though about use of hex, it's uncommon in the Fortran world.

Yes, changing to the Intel's syntax does require some refactor on the tools/shared/pragma.c. We need to add the attributes entry in pragmas & directives table, and in its handler we need to find the corresponding dtype according to pragma's argument and set its alignment value.

In fact, there is already align entry in pragmas & directives table, but just implementing nothing in its handler. So I reuse the
syntax !DIR$ ALIGN and achieve its function with a small amount of code.

If possible, I'd like to keep my original syntax, which allows me to do less work.

As for the use of hex, this patch supports both hexadecimal and decimal formats. And I have changed the test case to decimal formats and it works fine.

pawosm-arm · 2023-04-25T08:55:56Z

If possible, I'd like to keep my original syntax, which allows me to do less work.

It's ok for me.

JiaweiHawk · 2023-04-26T13:23:14Z

Hi @kiranchandramohan,

Thank @kiranchandramohan for reviewing.

We would like to support directives in commonly used formats. Is this based on some other compiler's directives? The only one I could find was Intel's which has the syntax !DIR$ ATTRIBUTES ALIGN : 64 :: R_alloc https://www.intel.com/content/www/us/en/docs/fortran-compiler/developer-guide-reference/2023-0/attributes-align.html

I chose this format only because I found the keyword "SW_ALIGN" in the source code, so I reused that part of the code, which resulted in this format. It is okay for me to use syntax !DIR$ ATTRIBUTES ALIGN : alignment :: object, I will refactor my patch to support this format.

If it is possible, can I still keep my original syntax?

It appears that Flang originally intended to implement !DIR$ align synatx, as there are align entry in pragmas & directives table in tools/shared/pragma.c, but no code in its handler.

So I think we can reuse this interface, and this can allow me to do less work.

kiranchandramohan · 2023-04-26T16:09:00Z

Hi @kiranchandramohan,

Thank @kiranchandramohan for reviewing.

We would like to support directives in commonly used formats. Is this based on some other compiler's directives? The only one I could find was Intel's which has the syntax !DIR$ ATTRIBUTES ALIGN : 64 :: R_alloc https://www.intel.com/content/www/us/en/docs/fortran-compiler/developer-guide-reference/2023-0/attributes-align.html

I chose this format only because I found the keyword "SW_ALIGN" in the source code, so I reused that part of the code, which resulted in this format. It is okay for me to use syntax !DIR$ ATTRIBUTES ALIGN : alignment :: object, I will refactor my patch to support this format.

If it is possible, can I still keep my original syntax?

It appears that Flang originally intended to implement !DIR$ align synatx, as there are align entry in pragmas & directives table in tools/shared/pragma.c, but no code in its handler.

So I think we can reuse this interface, and this can allow me to do less work.

If we are using a non-standard format then we will have to define the syntax, meaning, restrictions (which values can be provided, which types apply). Also, we will be setting a precedent and if the format is not available in llvm/flang then users might face issues in porting.

Having said that, we do not want to spend too much time adding the feature to Classic Flang. So I am OK if @bryanpkc and @shivaramaarao are OK. @pawosm-arm has already given his go ahead.

bryanpkc · 2023-05-17T10:44:31Z

So I am OK if @bryanpkc and @shivaramaarao are OK. @pawosm-arm has already given his go ahead.

The syntax looks fine to me. I will find time to review the PR.

tools/flang1/flang1exe/dtypeutl.c

tools/flang1/flang1exe/exterf.c

tools/flang1/flang1exe/lowersym.c

tools/flang1/flang1exe/exterf.c

tools/flang1/utils/symtab/sharedefs.h

tools/flang2/flang2exe/cgmain.cpp

tools/flang2/flang2exe/dtypeutl.cpp

JiaweiHawk · 2023-12-01T09:41:37Z

Hi all,

It seems like there are some issues with the CI.
CI encountered an error during the "Download artifacts" step, the complete log is provided below:

cd ../..
  curl -sL https://api.github.com/repos/flang-compiler/classic-flang-llvm-project/actions/workflows/pre-compile_llvm.yml/runs --output runs_llvm.json
  wget --output-document artifacts_llvm `jq -r '.workflow_runs[0].artifacts_url?' runs_llvm.json`
  
  i="0"
  # Keep checking older builds to find the right branch and correct number of artifacts
  while { [ `jq -r '.total_count?' artifacts_llvm` != "[5](/~https://github.com/flang-compiler/flang/actions/runs/7057653268/job/19213386208?pr=1360#step:5:5)" ] || \
      [ `jq -r --argjson i "$i" '.workflow_runs[$i].head_branch?' runs_llvm.json` != release_15x ]; } && \
      [ $i -lt 10 ];
  do
    echo "No artifacts or wrong branch in build $i, counting from latest"
    i=$[$i+1]
    wget --output-document artifacts_llvm `jq -r --argjson i "$i" '.workflow_runs[$i].artifacts_url?' runs_llvm.json`
  done
  url=`jq -r '.artifacts[] | select(.name == "llvm_build_X8[6](/~https://github.com/flang-compiler/flang/actions/runs/7057653268/job/19213386208?pr=1360#step:5:6)_gcc_[11](/~https://github.com/flang-compiler/flang/actions/runs/7057653268/job/19213386208?pr=1360#step:5:11)_release_15x") | .archive_download_url' artifacts_llvm`
  wget --output-document llvm_build.zip --header="Authorization: ***" $url
  shell: /usr/bin/bash -e {0}
  env:
    install_prefix: /usr/local

[...]

--2023-12-01 09:33:08--  https://api.github.com/repos/flang-compiler/classic-flang-llvm-project/actions/artifacts/888723439/zip
Resolving api.github.com (api.github.com)... 140.[82](/~https://github.com/flang-compiler/flang/actions/runs/7057653268/job/19213386208?pr=1360#step:5:83).114.5
Connecting to api.github.com (api.github.com)|140.82.114.5|:443... connected.
HTTP request sent, awaiting response... 410 Gone
2023-12-01 09:33:08 ERROR 410: Gone.

Error: Process completed with exit code 8.

pawosm-arm · 2023-12-04T13:20:18Z

@bryanpkc I think we need to make decisions here

if the failing test builds are result of the CI issues, could we merge this one without seeing it passing CI tests?
there are 14 commits in this PR, should we consider "Squash and merge" instead of "Rebase and merge"? This would ease any potential cherry-picks or reverts of this feature.

bryanpkc

I believe that adding a palign field to symbols in the ILM output necessitates bumping up the ILM version number in tools/flang2/flang2exe/upper.h.

JiaweiHawk · 2023-12-14T00:58:00Z

I believe that adding a palign field to symbols in the ILM output necessitates bumping up the ILM version number in tools/flang2/flang2exe/upper.h.

Thanks @bryanpkc for the review.

I've identified the similar issue in tools/flang1/flang1exe/lower.h, and I'll address that as well.

Additionally, I would like to inquire how to generate the numeric values on the left side of the comments, it seems that there isn't much information on how these numbers should be generated.

/*
 * 20.1         -- 1.55
 *              All of 1.54 +
 *              pass elemental field for subprogram when emitting ST_ENTRY.
 *
 *              For ST_PROC, receive IS_PROC_PTR_IFACE flag.
 *
 * ${how to generate this number}         -- 1.56
 *              All of 1.55 + PALIGN
 */

#include "gbldefs.h"
#include "semant.h"

#define VersionMajor 1
#define VersionMinor 56

bryanpkc · 2023-12-14T02:36:44Z

Additionally, I would like to inquire how to generate the numeric values on the left side of the comments, it seems that there isn't much information on how these numbers should be generated.

@JiaweiHawk I think those numbers are nvfortran version numbers, and are no longer relevant in open-source Classic Flang. You could use something like 23.12 to represent December 2023.

JiaweiHawk · 2023-12-14T02:53:47Z

Additionally, I would like to inquire how to generate the numeric values on the left side of the comments, it seems that there isn't much information on how these numbers should be generated.

@JiaweiHawk I think those numbers are nvfortran version numbers, and are no longer relevant in open-source Classic Flang. You could use something like 23.12 to represent December 2023.

Thank you for explanation. I will update this patchset according to your suggestions.

This patch adds the `palign` field in `struct SYM` to record the symbol's alignment passed by `!DIR$ ALIGN alignment`.

flang should store the alignment from `!DIR$ ALIGN alignment` pragma to the symbol when decalring a variable's symbol, so later flang can align the variable by ALIGNG(sptr).

flang1 uses export_symbol() to export symbols to .mod files, and uses import() to import symbols from .mod files. This patch exports `palign` field from `SYM` to .mod files, and imports `palign` field from .mod files to `ps->sym.palign`.

flang1 uses lower_symbol() to lower symbols to .stb file, and flang2 uses read_symbol() to upper symbols from .stb file. This patch exports `palign` field from `SYM` to .stb files, and imports `palign` field from .stb files to `SYM`. What's more, this patch also refactors a test case to pass regression tests.

To align the symbol set by `!DIR$ ALIGN alignment` pragma in flang1, flang should align both its symbol's offset in AG and AG's alignment in memory. This patch ensures that the BSS is aligned in memory to the maximum alignment among all symbols in the BSS.

To align the symbol set by `!DIR$ ALIGN alignment` pragma in flang1, flang should align both its symbol's offset in AG and AG's alignment in memory. This patch ensures that the statics is aligned in memory to the maximum alignment among all symbols in the statics.

To align the symbol set by `!DIR$ ALIGN alignment` pragma in flang1, flang should align both its symbol's offset in AG and AG's alignment in memory. This patch ensures that the cmn is aligned in memory to the maximum alignment among all symbols in the cmn.

To align the symbol set by `!DIR$ ALIGN alignment` pragma in flang1, flang should align both its symbol's offset in AG and AG's alignment in memory. This patch ensures the alignment of the symbol's offset in AG.

flang uses alignment_of_var()/alignment_var()/align_of_var() to get the symbol alignment. This patch fixes symbol alignment with `PALIGNG(s)` passed from pragma `!DIR$ ALIGN alignment` in flang1.

Add test case to validate the effectiveness of the pragma `!DIR$ ALIGN alignment` for derived types.

Add test case to validate the effectiveness of the pragma `!DIR$ ALIGN alignment` for array.

Add test case to validate the effectiveness of the pragma `!DIR$ ALIGN alignment` for character.

JiaweiHawk · 2023-12-14T10:07:46Z

Hi all,

It seems like CI does not find the clang binary.

build_flang (AArch64, release_15x) CI encountered an error during the "Check tools" step, the complete log is provided below:

Run git --version
git version 2.25.1
cmake version 3.20.0

CMake suite maintained and supported by Kitware (kitware.com/cmake).
GNU Make 4.2.1
Built for aarch64-unknown-linux-gnu
Copyright (C) 1988-2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
/__w/_temp/600f1194-c66d-4760-ac8f-f663a73d066f.sh: 4: /home/github/usr/local/bin/clang: not found
Error: Process completed with exit code 127.

bryanpkc · 2023-12-14T14:12:32Z

It seems like CI does not find the clang binary.

Yep, I became aware of the CI failures yesterday and have been trying to fix them. The cause is that the release_15x container image had expired and been deleted by GitHub. I will regenerate the image and rerun the workflow for this PR.

bryanpkc

LGTM. (Sorry I had re-requested a review from you by mistake, @pawosm-arm.)

pawosm-arm

suggested squashing the commits on merge

bryanpkc · 2023-12-15T00:32:03Z

Thank you very much for your contribution, @JiaweiHawk!

JiaweiHawk · 2023-12-15T00:46:33Z

Thank you very much for your contribution, @JiaweiHawk!

Thanks @bryanpkc and all the other reviewers for their help in getting this patchset finally merged!

JiaweiHawk force-pushed the align_pragma branch from a352e72 to f161447 Compare April 9, 2023 13:07

kiranchandramohan requested review from bryanpkc, pawosm-arm and shivaramaarao April 13, 2023 14:04

pawosm-arm reviewed Apr 15, 2023

View reviewed changes

test/directives/align_character_type.f90 Outdated Show resolved Hide resolved

JiaweiHawk force-pushed the align_pragma branch from f161447 to 5863814 Compare April 21, 2023 14:38

pawosm-arm reviewed Apr 21, 2023

View reviewed changes

test/directives/align_array_type.f90 Outdated Show resolved Hide resolved

JiaweiHawk force-pushed the align_pragma branch 2 times, most recently from b3a5219 to 05667b8 Compare April 25, 2023 02:36

JiaweiHawk force-pushed the align_pragma branch from 05667b8 to 72de48c Compare April 25, 2023 07:43