Page MenuHomePhabricator

[GuardWidening] Support widening of explicitly expressed guards
ClosedPublic

Authored by mkazantsev on Dec 25 2018, 2:43 AM.

Details

Summary

This patch adds support of guards expressed in explicit form via widenable_condition
in Guard Widening pass.

Diff Detail

Repository
rL LLVM

Event Timeline

mkazantsev marked an inline comment as done.Dec 25 2018, 9:40 PM
mkazantsev added inline comments.
test/Transforms/GuardWidening/basic_explicit_guards.ll
670 ↗(On Diff #179489)

Actually I don't understand why we think that widening here is profitable in intrinsic case. Not sure if we should do it.

mkazantsev marked an inline comment as done.Dec 25 2018, 10:03 PM
mkazantsev added inline comments.
test/Transforms/GuardWidening/basic_explicit_guards.ll
886 ↗(On Diff #179489)

I've re-read the test carefully. We don't do it in intrinsic case and shouldn't. TODO is to be removed.

mkazantsev marked an inline comment as done.Dec 25 2018, 10:28 PM
mkazantsev added inline comments.
lib/Transforms/Scalar/GuardWidening.cpp
495 ↗(On Diff #179489)

Actually the problem is in PDT with infinite loops. This comment needs removed.

Updated tests.

reames requested changes to this revision.Jan 16 2019, 5:26 PM
reames added inline comments.
lib/Transforms/Scalar/GuardWidening.cpp
87 ↗(On Diff #179498)

As noted elsewhere, I dislike the explicit terminology. I'd suggest calling this guard-widening-include-widenable-conditions.

That naming does imply something slightly more general though, see further comments.

101 ↗(On Diff #179498)

I really don't like the coupling between the single and matcher and the caller. What if we later extend the matcher to (v1 and (v2 and WC))?

402 ↗(On Diff #179498)

Not sure this check is needed?

484 ↗(On Diff #179498)

Having this adjust only one of the two variables looks awfully suspicious?

581 ↗(On Diff #179498)

No, just no. You've screwed up your abstraction here. WideanbleCondition had better be either Cond1 or Cond2 for this function to have reasonable semantics.

This revision now requires changes to proceed.Jan 16 2019, 5:26 PM
mkazantsev marked 3 inline comments as done.Jan 22 2019, 4:03 AM
mkazantsev added inline comments.
lib/Transforms/Scalar/GuardWidening.cpp
101 ↗(On Diff #179498)

What is the benefit of that? When a guard looks like br((A & B) & WC()), it is possible to strip the widenable condition from it, getting (A & B). When we have something like br((A & WC) & B)`, stripping WC would need to create instructions that don't currently exist. I would better stick to one canonical form, the more that it is easy to sustain.

402 ↗(On Diff #179498)

The logic here is a bit messy, I'll try to find a way to refactor the code around to go without it if possible.

484 ↗(On Diff #179498)

We can widen *any* condition (in theory even not a condition from guard) into a dominating guard. Here we require that the dominating guard is what we are trying to support, and the dominated instruction can be anything with condition. Maybe the renaming in underlying code would make sense.

mkazantsev updated this revision to Diff 185017.Feb 4 2019, 2:59 AM

Refactored underlying code to get rid of non-obvious things in logic. Rebased.

reames requested changes to this revision.Feb 4 2019, 11:28 AM

A few minor tweaks left, but rapidly approaching ready to land.

lib/Transforms/Scalar/GuardWidening.cpp
535 ↗(On Diff #185017)

This bit of code would make a lot more sense in the caller widenGuard - it's only needed by one of two callers - and would require a lot less code as well.

402 ↗(On Diff #179498)

See my response to one of your NFC cleanups on how this can be removed entirely.

test/Transforms/GuardWidening/basic_explicit_guards.ll
5 ↗(On Diff #185017)

This declaration is unused. Can you fix that by adding at least two tests mixing guards and widenable conditions? (i.e. one each widening from the other)

This revision now requires changes to proceed.Feb 4 2019, 11:28 AM
mkazantsev marked an inline comment as done.Feb 6 2019, 1:33 AM
mkazantsev added inline comments.
test/Transforms/GuardWidening/basic_explicit_guards.ll
5 ↗(On Diff #185017)

These tests have already be derived from test/Transforms/GuardWidening/basic.ll Does it make sense to duplicate?

mkazantsev updated this revision to Diff 185517.Feb 6 2019, 3:00 AM
mkazantsev marked an inline comment as done.

Housted logic from widenCondCommon to widenGuard.

mkazantsev planned changes to this revision.Feb 6 2019, 3:00 AM
mkazantsev updated this revision to Diff 185522.Feb 6 2019, 3:38 AM

Removed redundant code by an NFC, rebased.

mkazantsev marked an inline comment as done.Feb 6 2019, 4:01 AM
mkazantsev added inline comments.
test/Transforms/GuardWidening/basic_explicit_guards.ll
5 ↗(On Diff #185017)

i.e. these tests are directly derived from tests from basic.ll using MakeGuardsExplicit pass. I'm not sure that it makes sense to have them in two files at the same time.

reames accepted this revision.Feb 11 2019, 8:32 AM

LGTM w/two required follow ups. Can be before or after landing.

lib/Transforms/Scalar/GuardWidening.cpp
275 ↗(On Diff #185522)

minor: We're not changing toWiden, so this can be done as a single check below the call to widenCondCommon

test/Transforms/GuardWidening/basic_explicit_guards.ll
5 ↗(On Diff #185017)

Max, I can't follow your response to my comment here. I'm asking for a test mixing guards and wideable conditions. That is not tested elsewhere. I don't care which file you put the tests in provided you add them.

This revision is now accepted and ready to land.Feb 11 2019, 8:32 AM
mkazantsev marked an inline comment as done.

Added test with mixed guards (both intrinsic and widenable conditions).

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2019, 1:59 AM