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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 5 inline comments as done.
jdoerfert added a comment.

Addressed most comments, some I replied back to. Let me know if I should change those things as well.



================
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
----------------
chandlerc wrote:
> 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 ...
> ```
Used a script to move the description to the test cases (there might be more to come in other patches but I mention it because the format/wording might be a bit off now).

I don't get the back-tick part though.


================
Comment at: llvm/test/Transforms/FunctionAttrs/arg_returned.ll:34
+;
+; __attribute__((noinline)) int sink_r0(int r) {
+;   return r;
----------------
chandlerc wrote:
> 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.
I actually copy the code out of test cases quite frequently to run them in isolation or create a derived test case. Copy, paste, remove the first column, and I'm done.

The codes here are auto-formated and inserted by my "create_ll" script from the actual sources.
If it is OK, i'd prefer to keep it like this, if ppl feel strongly I can change it where requested.


================
Comment at: llvm/test/Transforms/FunctionAttrs/read_write_returned_arguments_scc.ll:28
+;   6 - Number of arguments marked readonly
+;   6 - Number of arguments marked returned
+;
----------------
This should not get out of date as it is the best I could derive for the test case. I removed the "stats" formating though.


================
Comment at: llvm/test/Transforms/FunctionAttrs/read_write_returned_arguments_scc.ll:34
+; CHECK-NEXT: define i32* @external_ret2_nrw(i32* %n0, i32* %r0, i32* %w0)
+define i32* @external_ret2_nrw(i32* %n0, i32* %r0, i32* %w0) {
+entry:
----------------
`dso_local` was aut-generated when my script created the test from C source. I'll remove them (in the future automatically) and for now by hand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59903