Page MenuHomePhabricator

Tests for nosync function attribute.
ClosedPublic

Authored by sstefan1 on May 24 2019, 1:36 PM.

Details

Reviewers
jdoerfert
Summary

This commit is test for upcoming patch on nosync attribute deduction

Event Timeline

sstefan1 created this revision.May 24 2019, 1:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2019, 1:36 PM

Some comments inlined. Please also add:

  • SCC tests with multiple functions calling each other. You don't need them for all kinds of sync operations but a positive and a negative test with volatile would be good.
  • Test with calls to declared functions. Positive (with nosync attribute present) and negative again.
llvm/test/Transforms/FunctionAttrs/nosync.ll
45

Remove the dso_local and local_unamed_addr everywhere

71

Maybe mention that we should *not* deduce nosync in the negative test cases.

117

We don't need these

sstefan1 updated this revision to Diff 201428.May 25 2019, 4:22 PM
  • updated tests for nosync.
sstefan1 marked 3 inline comments as done.EditedMay 25 2019, 4:26 PM

Addressed most of the comments.

a positive and a negative test with volatile would be good.

What can be the positive case for SCC tests with volatile? Isn't volatile always supposed to sync?

sstefan1 updated this revision to Diff 201438.May 26 2019, 1:33 AM
  • typo in test 10.

Addressed most of the comments.

a positive and a negative test with volatile would be good.

What can be the positive case for SCC tests with volatile? Isn't volatile always supposed to sync?

Good point. Ignore that part.

jdoerfert added inline comments.May 27 2019, 11:00 AM
llvm/test/Transforms/FunctionAttrs/nosync.ll
71

Add a
CHECK-NOT: nosync
after the check line because you could otherwise have nosync at the end and pass anyway.

117

FIXME: nosync missing
instead of nosync in the attribute list

We should be able to pass the test right now so we can integrate it already.
To this end, please also use the string attribute syntax "nosync" which should be accepted right now. (You can run lit on the test)

I'm now also unsure if a Release build will print the definitions are you check for them, that is the attributes listed at the end.
I think the form below should always work.

; CHECK:      Function Attrs: noinline nounwind uwtable
; CHECK-NEXT: define void @scc2(i32*)
sstefan1 updated this revision to Diff 201592.May 27 2019, 3:32 PM
  • updated tests. Will run lit first thing in the morning.
jdoerfert added inline comments.May 27 2019, 3:50 PM
llvm/test/Transforms/FunctionAttrs/nosync.ll
76

This is my bad but I think the combination of my prior to requests will not work.

Could you see if

; CHECK: Function Attrs: norecurse nounwind uwtable
; CHECK-NOT: nosync
; CHECK-NEXT: define i32 @load_acquire(i32* nocapture readonly)

works and if not remove the NEXT part.

sstefan1 updated this revision to Diff 201770.EditedMay 28 2019, 2:59 PM
  • Fixed tests. Removed some 'CHECK-NEXT' directives.
  • Test now passing.

CHECK-NEXT was the problem in TEST 8 & 9 so I removed it only from those two. Is that ok?

  • Fixed tests. Removed some 'CHECK-NEXT' directives.
  • Test now passing.

CHECK-NEXT was the problem in TEST 8 & 9 so I removed it only from those two. Is that ok?

What was the problem there? How does the output actually look?

What was the problem there? How does the output actually look?

nosync.ll:130:15: error: CHECK-NEXT: is not on the line after the previous match
; CHECK-NEXT: define void @call_nosync_function()
          ^
<stdin>:55:1: note: 'next' match was here
define void @call_nosync_function() #2 {
^
<stdin>:51:44: note: previous match ended here
; Function Attrs: noinline nounwind uwtable
                                       ^
<stdin>:52:1: note: non-matching line after previous match is here
declare void @nosync_function() #2
^

What was the problem there? How does the output actually look?

nosync.ll:130:15: error: CHECK-NEXT: is not on the line after the previous match
; CHECK-NEXT: define void @call_nosync_function()
          ^
<stdin>:55:1: note: 'next' match was here
define void @call_nosync_function() #2 {
^
<stdin>:51:44: note: previous match ended here
; Function Attrs: noinline nounwind uwtable
                                       ^
<stdin>:52:1: note: non-matching line after previous match is here
declare void @nosync_function() #2
^

Good thing we looked into this. You should look at the IR output (without lit) to see what the problem is but I would assume the decleration of nosync_function is placed before call_nosync_function. Then, the CHECK line you added for the attributes of the latter actually matches the attributes of the former. That would mean, adding another check line for the former, including the check-not nosync, will fix the issue and check-next will do what we want.

sstefan1 updated this revision to Diff 203467.Jun 6 2019, 4:38 PM

fixed the problem from last comment.

This revision is now accepted and ready to land.Jun 6 2019, 6:41 PM
jdoerfert closed this revision.Sep 11 2019, 4:48 PM

Afaik, this is in tree.