This is an archive of the discontinued LLVM Phabricator instance.

[NFC][FnAttrs] Stress tests for attribute deduction
ClosedPublic

Authored by jdoerfert on Mar 27 2019, 2:42 PM.

Details

Summary

This commit is a preparation of upcoming patches on attribute deduction.
It will shorten the diffs and make it clear what we inferred before.

Event Timeline

jdoerfert created this revision.Mar 27 2019, 2:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2019, 2:42 PM
Herald added a subscriber: bollu. · View Herald Transcript
jdoerfert updated this revision to Diff 192555.Mar 27 2019, 6:57 PM

Add two more test cases, exposed a bug

Add more tests and fix misleading comments

Add bug-exposing test case

jdoerfert updated this revision to Diff 193112.Apr 1 2019, 9:51 AM

Improve bug-exposing test cases (see http://llvm.org/PR41328)

sanjoy removed a reviewer: sanjoy.Apr 1 2019, 11:08 AM
chandlerc requested changes to this revision.Apr 17 2019, 3:12 AM
chandlerc added a subscriber: chandlerc.

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

llvm/test/Transforms/FunctionAttrs/SCC1.ll
2

Can we get a more helpful name than "SCC1"? I also prefer snake_case.ll to avoid any confusion w/ case insensitive filesystems.

8

nit: the mixture of * placement in this code (some on the left, some on the right) makes me twitch a little....

21–45

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.

50

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.

llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll
6–22 ↗(On Diff #193112)

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 ...
100 ↗(On Diff #193112)

Some judicious use of line breaks would make this more readable. Or use a variable the same way IR does.

llvm/test/Transforms/FunctionAttrs/arg_returned.ll
35

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.

This revision now requires changes to proceed.Apr 17 2019, 3:12 AM
jdoerfert updated this revision to Diff 195665.Apr 17 2019, 7:50 PM

Changes according to most of Chandler's comments

jdoerfert marked 5 inline comments as done.Apr 17 2019, 8:05 PM

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

llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll
6–22 ↗(On Diff #193112)

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.

llvm/test/Transforms/FunctionAttrs/arg_returned.ll
35

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.

llvm/test/Transforms/FunctionAttrs/read_write_returned_arguments_scc.ll
28 ↗(On Diff #195665)

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.

34 ↗(On Diff #195665)

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.

jdoerfert marked 3 inline comments as done.Apr 25 2019, 7:35 AM
jdoerfert added inline comments.
llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll
218 ↗(On Diff #195665)

I'll fix the CHECK lines, happens when you use search & replace -.-

327 ↗(On Diff #195665)

The CHECK line is broken

341 ↗(On Diff #195665)

Same as above

FIX some check lines

jdoerfert marked 3 inline comments as done.May 27 2019, 11:37 AM
jdoerfert added inline comments.
llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll
322 ↗(On Diff #201312)

Function name will be fixed, below the same.

llvm/test/Transforms/FunctionAttrs/arg_returned.ll
151

Description will be improved.

312

NoInlineNoRecurseNoUnwindUwtable will be removed (below as well)

jdoerfert updated this revision to Diff 201604.May 27 2019, 8:37 PM

Minor improvements

jdoerfert updated this revision to Diff 202636.Jun 2 2019, 7:50 PM

Fix small issues in one test

jdoerfert marked 2 inline comments as done.Jun 4 2019, 1:57 PM
hfinkel accepted this revision.Jun 4 2019, 3:44 PM

LGTM

This revision was not accepted when it landed; it landed in state Needs Review.Jun 4 2019, 7:59 PM
This revision was automatically updated to reflect the committed changes.