On 8/31/20 10:33 AM, Richard Biener wrote:
On Mon, Aug 31, 2020 at 10:20 AM Aldy Hernandez <al...@redhat.com> wrote:
As discussed in the PR, the type of a switch operand may be different
than the type of the individual cases. This is causing us to attempt to
intersect incompatible ranges.
This inconsistent IL seems wrong to me, but it also seems pervasive
throughout GCC. Jason, as one of the original gimple designers, do you
remember if this was an oversight, or was there a reason for this?
Richard, you mention in the PR that normalizing the IL would cause
propagation of widening cast sources into switches harder. Couldn't we
just cast all labels to the switch operand type upon creation? What
would be the problem with that? Would we generate sub-optimal code?
In either case, I'd hate to leave trunk in a broken case while this is
being discussed. The attached patch fixes the PRs.
OK?
widest_irange label_range (CASE_LOW (min_label), case_high);
+ if (!types_compatible_p (label_range.type (), range_of_op->type ()))
+ range_cast (label_range, range_of_op->type ());
label_range.intersect (range_of_op);
so label_range is of 'widest_int' type but then instead of casting
range_of_op to widest_irange you cast that widest_irange to the
type of the range op? Why not build label_range in the type
of range_op immediately?
The "widest" in widest_irange does not denote the type of the range, but
the number of sub-ranges that can maximally fit. It has nothing to do
with the underlying type of its elements. Instead, it is supposed to
indicate that label_range can fit an infinite amount of sub-ranges. In
practice this is 255 sub-ranges, but we can adjust that if needed.
So, in the constructor:
widest_irange label_range (CASE_LOW (min_label), case_high);
...the type for the sub-ranges in label_range is the type of
CASE_LOW(min_label) and case_high. They must match IIRC.
I am working on an irange best practices document that should go into
detail on all this, and will post it later this week.
Using widest_int for both would have been obvious to me, you're
indicating that switch (type op) { case type2 lab: } is supposed to
match as (type)lab == op rather than (type2)op == lab. I think
the IL will be consistent in a way that sign-extending both
op and lab to a common larger integer type is correct
(thus using widest_int), but no truncation should happen here
(such trucation should be applied by the frontend).
I suppose we could convert both label_range and range_of_op to a wider
type and do the intersection on these results. Is there a tree type
that indicates a widest possible int?
In any case type mismatches here are of course unfortunate
and both more verification and documentation would be
nice. verify_gimple_switch only verifies all case labels
have the same type, the type of the switch argument is
not verified in any way against that type.
Is the verification good enough with what Jakub posted? I can adjust
the documentation, but first I'd like a nod from Jason that this was
indeed expected behavior.
Thanks.
Aldy
