This is an archive of the discontinued LLVM Phabricator instance.

[NPM][SimplifyCFGPass] For OptForFuzzing attribute, disable SimplifyCondBranch and FoldTwoEntryPHINode in NPM
AbandonedPublic

Authored by zequanwu on Oct 16 2020, 1:11 PM.

Diff Detail

Event Timeline

zequanwu created this revision.Oct 16 2020, 1:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2020, 1:11 PM
zequanwu requested review of this revision.Oct 16 2020, 1:11 PM
zequanwu added inline comments.Oct 16 2020, 1:12 PM
llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
280

Copied from here.

lebedev.ri requested changes to this revision.Oct 16 2020, 1:14 PM
lebedev.ri added a subscriber: lebedev.ri.

This is missing description and test coverage.
Why is this NPM specific?
The specific folds should be guarded with that F.hasFnAttribute(Attribute::OptForFuzzing) check i think.

This revision now requires changes to proceed.Oct 16 2020, 1:14 PM

Can you add a corresponding -passes=simplifycfg RUN line in the failing test?

Why is this NPM specific?

This is something the legacy pass did but was not properly ported to the NPM pass.

zequanwu updated this revision to Diff 298739.Oct 16 2020, 1:22 PM

update test.

Why is this NPM specific?

This is something the legacy pass did but was not properly ported to the NPM pass.

Right. But why is this PM-specific at all, why aren't the folds themselves guarded with that?

Why is this NPM specific?

This is something the legacy pass did but was not properly ported to the NPM pass.

Right. But why is this PM-specific at all, why aren't the folds themselves guarded with that?

Oh I see, maybe we don't even need SimplifyCondBranch and FoldTwoEntryPHINode in SimplifyCFGOptions, just check for the OptForFuzzing attribute at time of use.

zequanwu added a comment.EditedOct 16 2020, 1:34 PM

Why is this NPM specific?

This is something the legacy pass did but was not properly ported to the NPM pass.

Right. But why is this PM-specific at all, why aren't the folds themselves guarded with that?

It's from here: https://github.com/llvm/llvm-project/commit/cdd006eec9409923f9a56b9026ce2cb72e7b71dc. It was guarded by FoldTwoEntryPHINode and simplifyCondBranch.
As the commit message said, "This should function as any other SimplifyCFGOption rather than having
the transform check and specially consider the attribute itself.". I think it was intended to have SimplifyCFGOptions as a control for SimplifyCondBranch and FoldTwoEntryPHINode.

lebedev.ri resigned from this revision.Oct 20 2020, 11:56 AM

Why is this NPM specific?

This is something the legacy pass did but was not properly ported to the NPM pass.

Right. But why is this PM-specific at all, why aren't the folds themselves guarded with that?

Oh I see, maybe we don't even need SimplifyCondBranch and FoldTwoEntryPHINode in SimplifyCFGOptions, just check for the OptForFuzzing attribute at time of use.

I think the transforms themselves should be explicitly guarded with !F.hasFnAttribute(Attribute::OptForFuzzing),
but i don't feel strongly about it..

I'd say either revert https://github.com/llvm/llvm-project/commit/cdd006eec9409923f9a56b9026ce2cb72e7b71dc, or in FoldTwoEntryPHINode() check that the option is set and that the OptForFuzzing attribute is not set (same with simplifyCondBranch()). I guess I'd slightly prefer the second option as to not totally revert the original commit.

I'd say either revert https://github.com/llvm/llvm-project/commit/cdd006eec9409923f9a56b9026ce2cb72e7b71dc, or in FoldTwoEntryPHINode() check that the option is set and that the OptForFuzzing attribute is not set (same with simplifyCondBranch()). I guess I'd slightly prefer the second option as to not totally revert the original commit.

Why do we want to check the option and not OptForFuzzing? Doesn't they have the same truth value?

I'd say either revert https://github.com/llvm/llvm-project/commit/cdd006eec9409923f9a56b9026ce2cb72e7b71dc, or in FoldTwoEntryPHINode() check that the option is set and that the OptForFuzzing attribute is not set (same with simplifyCondBranch()). I guess I'd slightly prefer the second option as to not totally revert the original commit.

Why do we want to check the option and not OptForFuzzing? Doesn't they have the same truth value?

As of right now, yes, but the point of https://github.com/llvm/llvm-project/commit/cdd006eec9409923f9a56b9026ce2cb72e7b71dc was that in the future, some consumer of simplifyCFG() may want to set those options. I'm not against a complete revert of that change, but it does sort of make sense.

zequanwu updated this revision to Diff 299520.Oct 20 2020, 5:08 PM

address comment

aeubanks added inline comments.Oct 20 2020, 7:55 PM
llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
248–251

the changes in this file shouldn't be necessary any more

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2419 ↗(On Diff #299520)

Fn should never be null, ditto below

zequanwu added inline comments.Oct 21 2020, 11:34 AM
llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
248–251

Then this is pretty much the same as reverting the original commit. And Line 280-286 wouldn't be necessary either (LPM pass).

aeubanks added inline comments.Oct 21 2020, 12:38 PM
llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
248–251

This would allow turning off SimplifyCondBranch and FoldTwoEntryPHINode, but otherwise yes. Maybe we should just revert with arsenm's approval.

zequanwu added inline comments.Oct 21 2020, 5:14 PM
llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
248–251

I add a revert patch here: D89917

zequanwu abandoned this revision.Nov 17 2020, 2:37 PM