[PATCH] D59903: [NFC][FnAttrs] Stress tests for attribute deduction

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Generally really nice. Some suggestions on cleaning up comments and such in the tests.



================
Comment at: llvm/test/Transforms/FunctionAttrs/SCC1.ll:1
+; RUN: opt -S -functionattrs -enable-nonnull-arg-prop %s | FileCheck %s
+;
----------------
Can we get a more helpful name than "SCC1"? I also prefer `snake_case.ll` to avoid any confusion w/ case insensitive filesystems.


================
Comment at: llvm/test/Transforms/FunctionAttrs/SCC1.ll:7
+;
+; static int* internal_ret0_nw(int *n0, int *w0);
+; static int* internal_ret1_rw(int *r0, int *w0);
----------------
nit: the mixture of `*` placement in this code (some on the left, some on the right) makes me twitch a little....


================
Comment at: llvm/test/Transforms/FunctionAttrs/SCC1.ll:20-44
+; Currently we get the following statistics:
+;   1 functionattrs - Number of functions marked as norecurse
+;   6 functionattrs - Number of functions marked as nounwind
+;   1 functionattrs - Number of arguments marked nocapture
+;   1 functionattrs - Number of arguments marked readnone
+;   1 functionattrs - Number of arguments marked readonly
+;   1 functionattrs - Number of arguments marked returned
----------------
This will surely get out of date. How about adding a separate run of `opt` just printing out the stats so that you can check them here? You can still leave the desired stats for contrast, but will mean that the current ones are ensured accurate.


================
Comment at: llvm/test/Transforms/FunctionAttrs/SCC1.ll:49
+; CHECK: define dso_local i32* @external_ret2_nrw(i32* %n0, i32* %r0, i32* %w0) #[[NOUNWIND:[0-9]*]]
+define dso_local i32* @external_ret2_nrw(i32* %n0, i32* %r0, i32* %w0) {
+entry:
----------------
Why `dso_local`? Is that important?

Also, how do you feel about checking the comment string spelling out the attributes rather than having to use variables? That would (IMO) make the tests much easier to read and maintain going forward.

Not for this patch, but I'd also be interested in changing the IR printer to put these comments on the line after the `define` but before the first label for the entry block. This would let you use nice `CHECK-LABEL` sections to get more helpful error messages when one fails. If you're up for it, given the amount of work you're doing here, it might be worth trying to make that change.


================
Comment at: llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll:6-22
+; TEST  1: comparison against NULL
+; TEST  2: comparison against NULL in control flow
+; TEST  3: singleton SCC
+; TEST  4: singleton SCC with lots of nested recursive calls
+; TEST  5: SCC with various calls, casts, and comparisons agains NULL
+; TEST  6: call to external function, marked no-capture
+; TEST  7: call to external var-args function, marked no-capture
----------------
Keeping these here seems likely to get out of date. Also, numbering them seems likely to get out of date.

How about just adding this to the comment on each one and removing the number?

For example, but using `'`s instead of back-ticks to avoid breaking phab's fenced region parser....

```
; Test for comparison against null:
; '''
; int is_null_return(int *p) {
;   return p == 0;
; }
; '''
;
; FIXME: ...
; CHECK: ...
define ...
```


================
Comment at: llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll:100
+; int* srec16(int* a) {
+;   return srec16(srec16(srec16(srec16(srec16(srec16(srec16(srec16(srec16(srec16(srec16(srec16(srec16(srec16(srec16(srec16(a))))))))))))))));
+; }
----------------
Some judicious use of line breaks would make this more readable. Or use a variable the same way IR does.


================
Comment at: llvm/test/Transforms/FunctionAttrs/arg_returned.ll:34
+;
+; __attribute__((noinline)) int sink_r0(int r) {
+;   return r;
----------------
These can't be directly compiled easily anyways, so I think you can cheat some on syntax to make it easier to read. `[[noinline]]` etc. Some line breaks below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59903