[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: test/AST/ast-dump-openmp-atomic.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s
+
----------------
gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > I'm not a fan of ast-dump tests.  They are fragile, difficult to update, difficult to read (when they are 700 lines long), and it is unclear what the important parts are.
> > > 
> > > WDYT about converting them to unit tests?  See `clang/unittests/AST/StmtPrinterTest.cpp` for an example.
> > > They are difficult to update.
> > 
> > The updating part is true, for all of the clang tests,
> > they unlike llvm tests have no update scripts, so it's a major pain.
> > Here, it's is actually reasonably simple:
> > 1. prune old `// CHECK*` lines
> > 2. run the run line
> > 3. prepend each line of output with `// CHECK-NEXT: `
> > 4. scrub addresses, `s/0x[0-9a-f]+/0x{{.*}}/`
> > 5. scrub filepath prefix, again a simple string-replace
> > 6. prune everything after `TranslationUnitDecl` and before first `FunctionDecl`
> > 7. replace a few `// CHECK-NEXT: ` with `// CHECK: `
> > 8. append the final processed output to the test file. done.
> > 
> > It would not be too hard to write an update tool for this, but i've managed with a simple macro..
> > 
> > > They are fragile, difficult to read (when they are 700 lines long),
> > > and it is unclear what the important parts are.
> > 
> > This is kind-of intentional. I've tried to follow the llvm-proper approach of gigantic 
> > tests that are overly verbose, but quickly cement the state so **any** change **will**
> > be noticed. That has been a **very** successful approach for LLVM.
> > 
> > In fact, the lack of this ast-dumper test coverage was not helping
> > the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.
> > 
> > So no, i really don't want to //convert// the tests into a less verbose state.
> > 
> > I suppose i //could// //add// a more fine-grained tests, 
> > though readability is arguable. Here, it is obvious nothing is omitted,
> > and you can quickly see the `openmp_structured_block` by Ctrl+F'ing for it.
> > With more distilled tests, it's less obvious. (i'm not talking about `StmtPrinterTest`)
> > Here, it's is actually reasonably simple:
> 
> Unfortunately, an 8-step process is anything but simple.
> 
> > This is kind-of intentional. I've tried to follow the llvm-proper approach of gigantic 
> > tests that are overly verbose, but quickly cement the state so any change will
> > be noticed.
> 
> That's pretty much a definition of a "fragile test".  The tests should check what they intend to check.
> 
> > That has been a very successful approach for LLVM.
> 
> I don't think LLVM does this, the CHECK lines are for the most part manually crafted.
> 
> > In fact, the lack of this ast-dumper test coverage was not helping
> > the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.
> 
> If we need tests for AST dumper, we should write tests for it.  OpenMP tests should not double for AST dumper tests.
> 
>> Here, it's is actually reasonably simple:
> Unfortunately, an 8-step process is anything but simple.

Hah, what i was saying is that it is entirely automatable,
all steps are identical each time.

> That's pretty much a definition of a "fragile test". The tests should check what they intend to check.
> I don't think LLVM does this, the CHECK lines are for the most part manually crafted.

No.
I //know// that 'most' of the new/updated tests are not manual,
from looking at the tests in commits, it's not an empty statement.

```
llvm/test/CodeGen$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll | xargs grep "update_llc" | wc -l; done | sort -r -n -k 2,2
X86 1427
PowerPC 125
RISCV 90
...
```
```
llvm/test/CodeGen$ find X86/ -iname *.ll | wc -l
3370
llvm/test/CodeGen$ find PowerPC/ -iname *.ll | wc -l
1019
llvm/test/CodeGen$ find RISCV/ -iname *.ll | wc -l
105
```
So a half of X86 backend tests, and pretty much all RISCV backend tests.
```
llvm/test$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll | xargs grep "update_test" | wc -l; done | sort -r -n -k 2,2
Transforms 845
Analysis 17
CodeGen 6
Other 2
```
```
$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll | wc -l; done | sort -r -n -k 2,2
Transforms 4620
Analysis 804
```
So sure, by the numbers, 'most' aren't auto-generated.
It's a question of legacy mainly.

I've both added llvm tests, and clang tests.
llvm tests are a breeze, just come up with sufficient IR,
and run the script [verify that the CHECK is what you thought it is,
or tune IR until it is], and you're done. With clang (codegen mostly),
one needs to first come up with the test, and then with the check lines,
and be really careful not to misspell `CHECK`, or else you're suddenly
not testing anything. clang tests are at least 3x more time-costly,
and i suspect thus have much less coverage they could have.

> If we need tests for AST dumper, we should write tests for it. OpenMP tests should not double for AST dumper tests.

I'll take a look at `StmtPrinterTest`, but i'm not sure i agree it's better.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59214/new/

https://reviews.llvm.org/D59214