Page MenuHomePhabricator

[Attributor] Deducing existing nounwind attribute.
ClosedPublic

Authored by sstefan1 on Jun 15 2019, 9:34 AM.

Event Timeline

sstefan1 created this revision.Jun 15 2019, 9:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2019, 9:34 AM

Is this sufficient to replace the old nounwind deduction? Can you disable the current one to see if there is something we cannot deduce with this one?

llvm/include/llvm/Transforms/IPO/Attributor.h
654

Isn't ID = Attribute::NoUnwind sufficient?

sstefan1 marked an inline comment as done.Jun 18 2019, 11:25 AM

Is this sufficient to replace the old nounwind deduction? Can you disable the current one to see if there is something we cannot deduce with this one?

I already tried it on some of the other tests and it worked fine. I am going to that and test it some more, and update tomorrow morning.

llvm/include/llvm/Transforms/IPO/Attributor.h
654

Totally, my bad.

sstefan1 added a comment.EditedJun 19 2019, 11:13 AM

Is this sufficient to replace the old nounwind deduction? Can you disable the current one to see if there is something we cannot deduce with this one?

I already tried it on some of the other tests and it worked fine. I am going to that and test it some more, and update tomorrow morning.

Sorry for the late update. I ran all tests with -disable-nounwind-inference -attributor -attributor-disable=false and they pass. Is that enough?

I have now run the test-suite with -disable-nounwind-inference with attributor and functionattrs with nounwind, with attributor disabled. These are the results:

attributor.NumFnNoUnwind: 935 (-disable-nounwind-inference)
functionattrs.NumNoUnwind: 262 (-attributor-disable=true)

jdoerfert accepted this revision.Jun 24 2019, 7:52 AM

Is this sufficient to replace the old nounwind deduction? Can you disable the current one to see if there is something we cannot deduce with this one?

I already tried it on some of the other tests and it worked fine. I am going to that and test it some more, and update tomorrow morning.

Sorry for the late update. I ran all tests with -disable-nounwind-inference -attributor -attributor-disable=false and they pass. Is that enough?

I have now run the test-suite with -disable-nounwind-inference with attributor and functionattrs with nounwind, with attributor disabled. These are the results:

attributor.NumFnNoUnwind: 935 (-disable-nounwind-inference)
functionattrs.NumNoUnwind: 262 (-attributor-disable=true)

That looks good. Do you happen to know if and how many attributes are deduced by the funcattrs pass if we enable both?

Please run clang format on the Attributor.cpp and revert the non-functional changes, e.g., in the comments.

llvm/include/llvm/Transforms/IPO/Attributor.h
654

No worries, update it though ;)

This revision is now accepted and ready to land.Jun 24 2019, 7:52 AM
sstefan1 updated this revision to Diff 206328.Jun 24 2019, 4:43 PM
  • Small fix
sstefan1 updated this revision to Diff 206331.Jun 24 2019, 4:47 PM
  • clang-format

Is this sufficient to replace the old nounwind deduction? Can you disable the current one to see if there is something we cannot deduce with this one?

I already tried it on some of the other tests and it worked fine. I am going to that and test it some more, and update tomorrow morning.

Sorry for the late update. I ran all tests with -disable-nounwind-inference -attributor -attributor-disable=false and they pass. Is that enough?

I have now run the test-suite with -disable-nounwind-inference with attributor and functionattrs with nounwind, with attributor disabled. These are the results:

attributor.NumFnNoUnwind: 935 (-disable-nounwind-inference)
functionattrs.NumNoUnwind: 262 (-attributor-disable=true)

That looks good. Do you happen to know if and how many attributes are deduced by the funcattrs pass if we enable both?

Please run clang format on the Attributor.cpp and revert the non-functional changes, e.g., in the comments.

Ran clang-format, but it seems it didn't have any effect.

Numbers are the same when both are enabled.

Maybe I should commit this before nosync, since it looks like it may take few more days?

sstefan1 updated this revision to Diff 206809.Jun 27 2019, 2:56 AM
  • reformated the code
  • fixed tests
This revision was automatically updated to reflect the committed changes.