[flang][nfc] Move tests from intrinsic-procedures.f90 to dedicated files #927

banach-space · 2021-07-19T20:51:57Z

All new tests files have been extracted automatically from intrinsic-procedures.f90 with the following script (I had to do a few minor edits manually too):

#! /usr/bin/env bash

set -euo pipefail

set -x

INTRINSICS=(ABS
"ADJUSTL"
"ADJUSTR"
"AIMAG"
"AINT"
"ALL"
"ALLOCATED"
"ANINT"
"ANY"
"ASSOCIATED"
"BTEST"
"CPU_TIME"
"DBLE"
"DIM"
"DPROD"
"CEILING"
"CONJG"
"COUNT"
"FLOOR"
"IABS"
"IAND"
"IBCLR"
"IBITS"
"IBSET"
"ICHAR"
"IEOR"
"INDEX"
"IOR"
"ISHFT"
"ISHFTC"
"LEN"
"LEN_TRIM"
"LGE"
"MAXLOC"
"MAXVAL"
"MINLOC"
"MINVAL"
"MVBITS"
"NINT"
"MODULO"
"NOT"
"PRODUCT"
"REPEAT"
"RESHAPE"
"RRSPACING"
"SCAN"
"SIGN"
"SPACING"
"SPREAD"
"SQRT"
"SUM"
"TRANSER"
"TRANSFER"
"TRIM"
"TRANSPOSE"
"VERIFY"
)

LLVM_PROJECT_ROOT=$1
INTRINSIC_TEST_DIR=$LLVM_PROJECT_ROOT/flang/test/Lower/intrinsic-procedures-new
INPUT_TEST_FILE=$LLVM_PROJECT_ROOT/flang/test/Lower/intrinsic-procedures.f90

mkdir $INTRINSIC_TEST_DIR
for intrinsic in "${INTRINSICS[@]}"
do
  test_file=$INTRINSIC_TEST_DIR/$intrinsic".f90"
  touch $test_file
  printf "%s\n\n" "! RUN: bbc -emit-fir %s -o - | FileCheck %s" >> "$test_file"
  awk -v intr="$intrinsic" '$0 ~ intr {flag=1;next} /! [A-Z]+/ && !/! CHECK.*/{flag=0} flag' "$INPUT_TEST_FILE" >> "$test_file"
done

Please consider this patch as an RFC.

~~Also, I am aware that 2 tests are currently failing:~~

********************
********************
Failed Tests (2):
  Flang :: Lower/intrinsic-procedures-new/SPACING.f90
  Flang :: Lower/intrinsic-procedures-new/TRIM.f90


Testing Time: 6.14s
  Unsupported      :    1
  Passed           : 1034
  Expectedly Failed:    8
  Failed           :    2

~~I've not had the time to investigate this. I'd like to hear your thoughts first.~~

EDIT
Lower/intrinsic-procedures-new/SPACING.f90 was failing due to invalid CHECK lines, e.g.: https://github.com/flang-compiler/f18-llvm-project/blob/fir-dev/flang/test/Lower/intrinsic-procedures.f90#L1188 (missing : after CHECK).

Lower/intrinsic-procedures-new/TRIM.f90 was getting confused when matching the CHECK-LABEL directives. They are not unique enough and get matched match with code inside the functions.

I have fixed these in my 2nd commit.

jeanPerier

On my side that looks reasonable. I agree an intrinsic by intrinsic file split will help regression investigation and is the easiest to reason about.

If other people are OK with this change, could you rename the new directory to intrinsic-procedures (without the new suffix), it won't be new for long ! There is also a tutorial about intrinsic that would need a small update regarding where to write tests: (here and here).
Thanks !

kiranchandramohan

How difficult would it be to convert to lowercase file names (subject to someone else also asking for it)?

I have suggested some changes as questions.

flang/test/Lower/intrinsic-procedures/TRANSER.f90

flang/test/Lower/intrinsic-procedures/TRIM.f90

flang/test/Lower/intrinsic-procedures/RRSPACING.f90

flang/test/Lower/intrinsic-procedures/PRODUCT.f90

flang/test/Lower/intrinsic-procedures/LEN.f90

flang/test/Lower/intrinsic-procedures/ISHFTC.f90

flang/test/Lower/intrinsic-procedures/ANINT.f90

flang/test/Lower/intrinsic-procedures/ALL.f90

flang/test/Lower/intrinsic-procedures/AINT.f90

banach-space · 2021-07-20T17:04:03Z

How difficult would it be to convert to lowercase file names (subject to someone else also asking for it)?

Not difficult at all

#! /usr/bin/env bash

set -euo pipefail

LLVM_PROJECT_ROOT=$1
INTRINSIC_TEST_DIR=$LLVM_PROJECT_ROOT/flang/test/Lower/intrinsic-procedures

cd $INTRINSIC_TEST_DIR

for test_file in *
do
  dest_file=$(echo $test_file | tr 'A-Z' 'a-z')
  mv $test_file $dest_file
done

Just run like this:

bash rename.sh <f18-llvm-project-root-dir>

flang/test/Lower/intrinsic-procedures/TRANSFER.f90

mleair · 2021-07-21T18:46:46Z

Overall, looks good to me. As @jeanPerier stated, please update the intrinsic tutorial to indicate the new process for adding intrinsic tests.

banach-space · 2021-07-22T12:53:03Z

I have just rebased this on top of fir-dev. @mleair , the documentation should be up-to-date now.

@kiranchandramohan I'm a bit confused with the test in RRSPACING.f90. Any suggestions?

banach-space · 2021-07-23T13:12:14Z

Rebased on top of fir-dev. Added DOT_PRODUCT.f90.

kiranchandramohan

LGTM.

I still prefer lowercase file names. :)

schweitzpgi · 2021-07-27T14:47:27Z

LGTM.

I still prefer lowercase file names. :)

Lowercase file names would be more consistent with the rest of the tests.

schweitzpgi

Thanks for splitting this up.

I know you didn't write the tests initially, but it looks like some of the individual tests could be hardened here. More CHECK lines, more elaborated CHECK lines that include types, getting rid of CHECK-DAGs that don't seem to be necessary, etc.

flang/docs/tutorials/addingIntrinsics.md

flang/test/Lower/intrinsic-procedures/ABS.f90

flang/test/Lower/intrinsic-procedures/AIMAG.f90

flang/test/Lower/intrinsic-procedures/ALL.f90

flang/test/Lower/intrinsic-procedures/ANINT.f90

flang/test/Lower/intrinsic-procedures/COUNT.f90

flang/test/Lower/intrinsic-procedures/DIM.f90

flang/test/Lower/intrinsic-procedures/VERIFY.f90

banach-space · 2021-07-27T19:50:28Z

Hi @schweitzpgi ,

I know you didn't write the tests initially, but it looks like some of the individual tests could be hardened here. More CHECK lines, more elaborated CHECK lines that include types, getting rid of CHECK-DAGs that don't seem to be necessary, etc.

I agree. In general, I would rather do such refactoring in a separate patch. It would make for a cleaner Git history. I will try that next time - this patch already fixes quite a few tests (i.e. it's no longer a verbatim copy).

I've just uploaded a patch with updated CHECK lines. Mostly generated with generate-test-checks.py. I limited myself to the files that you pointed out. Could you quickly scan this commit? It's fairly late here and I might have missed something obvious.

banach-space · 2021-07-27T19:53:31Z

@schweitzpgi , @kiranchandramohan , the 4th commit that I've just uploaded renames this files as suggested.

flang/test/Lower/intrinsic-procedures/ALL.f90

schweitzpgi · 2021-07-29T14:12:04Z

Hi @schweitzpgi ,

I know you didn't write the tests initially, but it looks like some of the individual tests could be hardened here. More CHECK lines, more elaborated CHECK lines that include types, getting rid of CHECK-DAGs that don't seem to be necessary, etc.

I agree. In general, I would rather do such refactoring in a separate patch. It would make for a cleaner Git history. I will try that next time - this patch already fixes quite a few tests (i.e. it's no longer a verbatim copy).

I've just uploaded a patch with updated CHECK lines. Mostly generated with generate-test-checks.py. I limited myself to the files that you pointed out. Could you quickly scan this commit? It's fairly late here and I might have missed something obvious.

I commented on a possible typo. Looks like you used the generate script with the VAL_<n> check lines. :)

I'm fine with processing the rest of the files with the script in a separate PR.

banach-space · 2021-07-29T15:28:24Z

Looks like you used the generate script with the VAL_<n> check lines. :)

Yes, after a few hundred hand-written lines you reach the "there must a better way" point :) Do you use it and does it work for you out of the box?

schweitzpgi · 2021-07-29T15:57:52Z

Looks like you used the generate script with the VAL_<n> check lines. :)

Yes, after a few hundred hand-written lines you reach the "there must a better way" point :) Do you use it and does it work for you out of the box?

I do use it. But, no, it doesn't always work right.

schweitzpgi · 2021-07-29T15:59:52Z

Can you squash the commits? Then we can get this merged in. Thanks, Andrzej.

All tests from flang/test/Lower/intrinsic-procedures.f90 are moved to dedicated files (with the exception of `lge`, `lgt`, `lle` and `llt`). With this change, every filename clearly defines which intrinsic procedure is being tested. After the initial split, we discovered that some tests were invalid (e.g. `! CHECK` was used instead of `! CHECK:`). These tests are now fixed. Some tests are additionally hardened (e.g. some `CHECK-DAG` directives was replaced with `CHECK`). Some tests are updated with checks generated with mlir/utils/generate-test-checks.py. In other words, this patch _moves_ and _reformats_ the tests. The documentation is updated accordingly (with some extra clarifications, as suggested by the reviewers). Please see the following PRs for a complete discussion: * #927 * #914

schweitzpgi · 2021-07-30T03:39:49Z

There appears to be a problem with intrinsic-procedures/verify.f90. I get a CHECK failure when I build with clang++ (head).

banach-space · 2021-07-30T09:24:48Z

There appears to be a problem with intrinsic-procedures/verify.f90. I get a CHECK failure when I build with clang++ (head).

I have not been able to reproduce this :/ Tried with Clang-12 and Clang-13.

I did have to tweak 2 values in verify.f90 (these changed between the builds, so I replaced them with regex patterns): https://github.com/flang-compiler/f18-llvm-project/blob/fir-dev/flang/test/Lower/intrinsic-procedures/verify.f90#L18-L19. The first one looks like an address, so I'm not surprised that it might change. The 2nd is a bit of mystery to me. Otherwise, I don't see anything suspicious.

All tests from flang/test/Lower/intrinsic-procedures.f90 are moved to dedicated files (with the exception of `lge`, `lgt`, `lle` and `llt`). With this change, every filename clearly defines which intrinsic procedure is being tested. After the initial split, we discovered that some tests were invalid (e.g. `! CHECK` was used instead of `! CHECK:`). These tests are now fixed. Some tests are additionally hardened (e.g. some `CHECK-DAG` directives was replaced with `CHECK`). Some tests are updated with checks generated with mlir/utils/generate-test-checks.py. In other words, this patch _moves_ and _reformats_ the tests. The documentation is updated accordingly (with some extra clarifications, as suggested by the reviewers). Please see the following PRs for a complete discussion: * #927 * #914

banach-space mentioned this pull request Jul 19, 2021

Add lowering for the DOT_PRODUCT intrinsic #914

Merged

banach-space requested review from jeanPerier, mleair and psteinfeld July 20, 2021 09:44

jeanPerier requested a review from schweitzpgi July 20, 2021 11:07

jeanPerier reviewed Jul 20, 2021

View reviewed changes

jeanPerier approved these changes Jul 20, 2021

View reviewed changes

kiranchandramohan reviewed Jul 20, 2021

View reviewed changes

mleair reviewed Jul 21, 2021

View reviewed changes

flang/test/Lower/intrinsic-procedures/TRANSFER.f90 Outdated Show resolved Hide resolved

kiranchandramohan approved these changes Jul 26, 2021

View reviewed changes

schweitzpgi reviewed Jul 27, 2021

View reviewed changes

schweitzpgi reviewed Jul 29, 2021

View reviewed changes

flang/test/Lower/intrinsic-procedures/ALL.f90 Outdated Show resolved Hide resolved

schweitzpgi merged commit 3cedc9c into flang-compiler:fir-dev Jul 30, 2021

banach-space deleted the andrzej/separate_tests branch July 1, 2022 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang][nfc] Move tests from intrinsic-procedures.f90 to dedicated files #927

[flang][nfc] Move tests from intrinsic-procedures.f90 to dedicated files #927

banach-space commented Jul 19, 2021 •

edited

Loading

jeanPerier left a comment

kiranchandramohan left a comment

banach-space commented Jul 20, 2021

mleair commented Jul 21, 2021

banach-space commented Jul 22, 2021

banach-space commented Jul 23, 2021

kiranchandramohan left a comment

schweitzpgi commented Jul 27, 2021

schweitzpgi left a comment

banach-space commented Jul 27, 2021

banach-space commented Jul 27, 2021

schweitzpgi commented Jul 29, 2021 •

edited

Loading

banach-space commented Jul 29, 2021

schweitzpgi commented Jul 29, 2021

schweitzpgi commented Jul 29, 2021

schweitzpgi commented Jul 30, 2021 •

edited

Loading

banach-space commented Jul 30, 2021

[flang][nfc] Move tests from intrinsic-procedures.f90 to dedicated files #927

[flang][nfc] Move tests from intrinsic-procedures.f90 to dedicated files #927

Conversation

banach-space commented Jul 19, 2021 • edited Loading

jeanPerier left a comment

Choose a reason for hiding this comment

kiranchandramohan left a comment

Choose a reason for hiding this comment

banach-space commented Jul 20, 2021

mleair commented Jul 21, 2021

banach-space commented Jul 22, 2021

banach-space commented Jul 23, 2021

kiranchandramohan left a comment

Choose a reason for hiding this comment

schweitzpgi commented Jul 27, 2021

schweitzpgi left a comment

Choose a reason for hiding this comment

banach-space commented Jul 27, 2021

banach-space commented Jul 27, 2021

schweitzpgi commented Jul 29, 2021 • edited Loading

banach-space commented Jul 29, 2021

schweitzpgi commented Jul 29, 2021

schweitzpgi commented Jul 29, 2021

schweitzpgi commented Jul 30, 2021 • edited Loading

banach-space commented Jul 30, 2021

banach-space commented Jul 19, 2021 •

edited

Loading

schweitzpgi commented Jul 29, 2021 •

edited

Loading

schweitzpgi commented Jul 30, 2021 •

edited

Loading