Page MenuHomePhabricator

[bugpoint][PR29027] Reduce function attributes
ClosedPublic

Authored by modocache on Dec 3 2018, 6:58 AM.

Details

Summary

In addition to reducing the functions in an LLVM module, bugpoint now
reduces the function attributes associated with each of the remaining
functions.

To test this, add a -bugpoint-crashfuncattr test pass, which crashes if
a function in the module has a "bugpoint-crash" attribute. A test case
demonstrates that the IR is reduced to just that one attribute.

Diff Detail

Repository
rL LLVM

Event Timeline

modocache created this revision.Dec 3 2018, 6:58 AM
reames added a subscriber: reames.Dec 3 2018, 3:49 PM

Looks like a good step in the right direction after some cleanup. Any chance you're interested in doing the same for argument or return attributes?

test/BugPoint/func-attrs.ll
6 ↗(On Diff #176395)

Can you add an example which uses key=value for the crashing attribute to show that works? "bugpoint-crash"="blah" should work fine.

11 ↗(On Diff #176395)

Can you turn this into a positive test for the one left?

tools/bugpoint/CrashDebugger.cpp
333 ↗(On Diff #176395)

It probably makes more sense to call this Suffix?

1142 ↗(On Diff #176395)

This should probably be outside the loop?

modocache updated this revision to Diff 177442.Dec 9 2018, 10:20 AM
modocache marked an inline comment as done.

Thanks for the review, @reames! I implemented each of your suggestions, but I had a small excuse for not using the variable name Suffix.

modocache marked 3 inline comments as done.Dec 9 2018, 10:21 AM
modocache added inline comments.
tools/bugpoint/CrashDebugger.cpp
333 ↗(On Diff #176395)

I agree, but every other implementation of doTest names this Kept... maybe we can rename them all in a future NFC commit?

Friendly ping! I believe I addressed all of the code review comments. @MatzeB, you created the bug report https://bugs.llvm.org/show_bug.cgi?id=29027, is this implementation what you had in mind?

reames accepted this revision.Dec 17 2018, 5:28 PM

LGTM, thanks for the ping!

This revision is now accepted and ready to land.Dec 17 2018, 5:28 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the reviews, @reames! As a follow-up to this, I also have https://reviews.llvm.org/D55601 out for review, if you're interested -- it improves the reduction logic added in this patch.