This patch adds support of guards expressed in explicit form via widenable_condition
in Guard Widening pass.
Details
Diff Detail
Event Timeline
test/Transforms/GuardWidening/basic_explicit_guards.ll | ||
---|---|---|
671 | Actually I don't understand why we think that widening here is profitable in intrinsic case. Not sure if we should do it. |
test/Transforms/GuardWidening/basic_explicit_guards.ll | ||
---|---|---|
887 | I've re-read the test carefully. We don't do it in intrinsic case and shouldn't. TODO is to be removed. |
lib/Transforms/Scalar/GuardWidening.cpp | ||
---|---|---|
491 | Actually the problem is in PDT with infinite loops. This comment needs removed. |
lib/Transforms/Scalar/GuardWidening.cpp | ||
---|---|---|
86 | 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. | |
100 | 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))? | |
403 | Not sure this check is needed? | |
480 | Having this adjust only one of the two variables looks awfully suspicious? | |
571 | 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. |
lib/Transforms/Scalar/GuardWidening.cpp | ||
---|---|---|
100 | 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. | |
403 | 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. | |
480 | 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. |
A few minor tweaks left, but rapidly approaching ready to land.
lib/Transforms/Scalar/GuardWidening.cpp | ||
---|---|---|
403 | See my response to one of your NFC cleanups on how this can be removed entirely. | |
537 | 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. | |
test/Transforms/GuardWidening/basic_explicit_guards.ll | ||
6 | 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) |
test/Transforms/GuardWidening/basic_explicit_guards.ll | ||
---|---|---|
6 | These tests have already be derived from test/Transforms/GuardWidening/basic.ll Does it make sense to duplicate? |
test/Transforms/GuardWidening/basic_explicit_guards.ll | ||
---|---|---|
6 | 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. |
LGTM w/two required follow ups. Can be before or after landing.
lib/Transforms/Scalar/GuardWidening.cpp | ||
---|---|---|
275 | 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 | ||
6 | 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. |
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.