On Thu, 13 Apr 2017, Peter Bergner wrote:
> Bah, fixing up my return address.
>
>
> On 4/13/17 3:14 AM, Richard Biener wrote:
> > To recap the situation (from what I can deciper out of the ppc asm
> > and the expand RTL) we seem to expand to
> >
> > if (cond > 2)
> > __builtin_unreachable (); // jumps to the jump table data(?)
> > goto *tbl[cond];
> >
> > now I do not remember the reason why we keep __builtin_unreachable ()
> > at the RTL level -- on GIMPLE we keep it to be able to extract
> > range information from the controlling condition.
>
> As Jakub said, we do remove the blocks. But we don't remove the branch
> to those blocks. That is true of whether the __builtin_unreachable() is
> in the default case statement or in a normal case statement. That is
> what leads to the wild branch issue.
>
>
> > Your patch comments suggest that handling of gaps is similarly
> > improved but the testcase doesn't contain any gaps.
>
> I didn't change the way the jump table gaps are handled. They have
> always been redirected to the "default label"... although the patch
> doesn't use default_label anymore, it uses gap_label, as resetting
> default_label to fallback_label when default_label is NULL was
> what was stopping us from removing the range check.
>
> What is changed, is that now we also handle case labels that point to
> blocks with __builtin_unreachable() and those are also redirected to
> the new gap_label.
>
> I did not include a test case with an unreachable case statement,
> because unlike the unreachable default case test case, there is no
> compare/branch I can verify has been removed. However, looking
> closer, it seems the unpatched compiler leaves the unreachable
> block as well as it's label in the jump table, so I should be able
> to create an executable test case that uses a switch condition
> that targets the unreachable case and see if we SEGV/SIGILL or not.
> I cannot do that for the unreachable default case, since the range
> check we remove is what saves us from accessing the jump table
> with an index that is outside of the table.
>
>
>
> > So I wonder why we don't simply apply the "transform" at the GIMPLE
> > level, for example in the kitchen-sink pass_fold_builtins. That is
> > remove all __builtin_unreachable () labels and if the default label
> > is amongst them make the first one the new default (or maybe the
> > last one dependent on which would shrink jump table size most).
>
> Well if the default case is unreachable and we compute a "new"
> default, then we won't be able to eliminate the range check
> (ie, compare/branch to default). So isn't it performance wise
> better to eliminate the range check, rather than choosing a
> new default? If you think we shouldn't care about that, then
> I'd vote for choosing the first/last label with the higher
> edge probability rather than trying to shrink the table.
True.
> > VRP2 removes the if because we put the range computed by VRP1
> > on the SSA name used in the test (we have special code handling
> > unreachable paths there). For your testcase with the switch
> > as cond_2 is not used after the switch we do not compute any
> > range for it, otherwise we'd likely eliminate the default
> > case as well. For the small testcase above fold-all-builtins
> > handles the removal as well if VRP is not run, so extending
> > that to handle switch stmts sounds reasonable (see
> > optimize_unreachable). There's even a TODO in this function.
>
> If we updated optimize_unreachable() to remove unreachable
> default cases, would we still be able to disambiguate between
> a switch statement written with no default case and one where
> the default case was explicitly shown to be unreachable?
No.
> Maybe the default_label would be NULL for the unreachable
> case and non-NULL in the other case? If so, we'd still need
> my change that doesn't set default_label to fallback_label
> and instead uses the new var gap_label.
That would be possible though we now rely on the default label
to be present IIRC.
Ok, so I think we should ensure that we remove the regular cases
with unreachable destination, like in
switch (i)
{
case 0:
__builtin_unreachable ();
default:;
}
and handle default with __builtin_unreachable () from switch
expansion by omitting the range check for the jump table
indexing (and choose a non-default case label for gaps).
Richard.
