This commit is a preparation of upcoming patches on attribute deduction.
It will shorten the diffs and make it clear what we inferred before.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 29835 Build 29834: arc lint + arc unit
Event Timeline
Comment Actions
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 | ||
| 7–23 | 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 ... | |
| 101 | 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. | |
Comment Actions
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 | ||
|---|---|---|
| 7–23 | 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. | |
| 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. |
Can we get a more helpful name than "SCC1"? I also prefer snake_case.ll to avoid any confusion w/ case insensitive filesystems.