Page MenuHomePhabricator

Remove EnableEarlyCSEMemSSA option
ClosedPublic

Authored by echristo on Apr 15 2019, 7:21 PM.

Details

Summary

This has been turned on for about a year and a half now and removing it helps simplify the pass structure (though not a lot).

I didn't see much of a review either way so I don't even know if people care or want it on by default as this is literally the only place that has it on by default and it appears to have been used for GVN Hoist which is still off by default.

Thoughts? Conditionalize it on GVN Hoist? Something else?

Diff Detail

Repository
rL LLVM

Event Timeline

echristo created this revision.Apr 15 2019, 7:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2019, 7:21 PM

Is this still useful for debugging problems with MemSSA and GVNHoist? Is this mirrored in the new pass manager?

I can't find any users of this flag, so I'm in favor of s/EnableEarlyCSEMemSSA/true/ if that simplifies things.

Is this still useful for debugging problems with MemSSA and GVNHoist?

If we just have to tweak this one place, IMO it's sufficiently straightforward to just manually flip between true/false in code. I've personally never used it to debug anything, though I can only speak for myself.

I can't find any users of this flag, so I'm in favor of s/EnableEarlyCSEMemSSA/true/ if that simplifies things.

Is this still useful for debugging problems with MemSSA and GVNHoist?

If we just have to tweak this one place, IMO it's sufficiently straightforward to just manually flip between true/false in code. I've personally never used it to debug anything, though I can only speak for myself.

That was pretty much my thought. As far as the new pass manager - the flag isn't being used anywhere else other than this location :)

I can't find any users of this flag, so I'm in favor of s/EnableEarlyCSEMemSSA/true/ if that simplifies things.

Is this still useful for debugging problems with MemSSA and GVNHoist?

If we just have to tweak this one place, IMO it's sufficiently straightforward to just manually flip between true/false in code. I've personally never used it to debug anything, though I can only speak for myself.

That was pretty much my thought. As far as the new pass manager - the flag isn't being used anywhere else other than this location :)

Except there's a subtly different one there. I'll also remove that.

-eric

echristo updated this revision to Diff 195490.Apr 16 2019, 5:06 PM

Update for the new pass manager as well. :)

This sounds like people are fine with me doing this...

This sounds like people are fine with me doing this...

Agreed.

Please wait until tomorrow before committing in case anyone wants to voice last-second concerns.

Thanks again!

This revision is now accepted and ready to land.Apr 18 2019, 1:52 PM
This revision was automatically updated to reflect the committed changes.