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

Tue Mar 12 13:54:28 PDT 2019

gribozavr 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
+
----------------
lebedev.ri wrote:
> 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.
> Hah, what i was saying is that it is entirely automatable, all steps are identical each time.

What about the issue of reviewing the resulting diffs?  How likely is that the programmer will notice an unintended change in the sea of diffs?

If my arguments don't convince you, here's some industry wisdom: https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html

Repository:
  rC Clang

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

https://reviews.llvm.org/D59214