Re: POWER __builtin_add_overflow/__builtin_mul_overflow with u64
Hi!
On Tue, Feb 14, 2023 at 09:23:55AM +0100, Jakub Jelinek wrote:
> CCing Segher and David on this.
> rs6000 indeed doesn't implement {,u}{add,sub,mul}v4_optab for
> any mode and thus leaves it to the generic code.
Yes. Can we do better than the generic code, for those?
> On Tue, Feb 14, 2023 at 04:48:42AM +0100, Simon Richter wrote:
> > I'm looking at the generated code for these builtins on POWER:
> >
> > add 4,3,4
> > subfc 3,3,4
> > subfe 3,3,3
> > std 4,0(5)
> > rldicl 3,3,0,63
> > blr
> >
> > and
> >
> > mulld 10,3,4
> > mulhdu 3,3,4
> > addic 9,3,-1
> > std 10,0(5)
> > subfe 3,9,3
> > blr
(The _overflow builtins, which obviously generate different code).
This is pretty much as good as we can do, at least with older ISAs.
With ISA 3.0 (p9) we have isel and with ISA 3.1 (p10) we have setbc*,
allowing us to improve on this slightly.
> > The POWER architecture has variants of these instructions with builtin
> > overflow checks (addo/mulldo), but these aren't listed in the .md files, and
> > the builtins don't generate them either.
> >
> > Is this intentional (I've found a few comments that mulldo is microcoded on
> > CellBE and should be avoided there)?
As Eric points at we cannot easily use OV since ISA 2.00 (p4, 2001).
ISA 3.0 allows us to use addex to get at the OV bit (but inconveniently,
the insn is really only meant to allow multiple regs in lon carry chains
for multi-precision arithmetic and the like), but nothing else does.
Before ISA 2.00 we could use OV easily using the mcrxr instruction, or
save up testing it for as many insns as we want using the SO bit, which
mcrxr also reads, and conveniently also clears. But instructions like
that (reading from three separate resoources and writing to one as well,
one that is not renamed even) are not suitable for heavily out-of-order
implementations.
Since ISA 3.0 we can read OV using mcrxrx. This can allow slightly
faster sequences for some specialised cases. That insn also allows us
to move CA into a GPR in just one insn as well, hrm :-)
I'll add a work item to investigate what we can do here. Improvements
will be only marginal, maybe an insn or a cycle or two can be saved
here or there, but it is not likely worth it to have o variants of most
instructions (which are very inconvenient to deal with in the compiler,
they are much nicer for human writers).
Segher
get_range_query vs NULL argument
Hi, While fixing PR 108354, I came across that ssa_name_has_boolean_range calls get_range_query with cfun as the argument but sometimes while in IPA passes cfun is currently nullptr. The question should we check the argument before calling get_range_query or is it a valid thing to call it with a nullptr (and have it return global_ranges in that case)? I committed the patch (for PR 108354) with the workaround around the issue via having the match.pd pattern which was calling ssa_name_has_boolean_range only be invoked from the gimple simplifiers. Below is the patch which undones the workaround: ``` diff --git a/gcc/match.pd b/gcc/match.pd index e7b700349a6..e352bd422f5 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -1732,7 +1732,6 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (if (!FIXED_POINT_TYPE_P (type)) (plus @0 (negate @1 -#if GIMPLE /* 1 - a is a ^ 1 if a had a bool range. */ /* This is only enabled for gimple as sometimes cfun is not set for the function which contains @@ -1743,7 +1742,6 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (if (INTEGRAL_TYPE_P (type) && ssa_name_has_boolean_range (@1)) (bit_xor @1 @0))) -#endif /* Other simplifications of negation (c.f. fold_negate_expr_1). */ (simplify ``` Note I can only so far reproduce the call to ssa_name_has_boolean_range that causes an issue while building Ada tools (while bootstrapping) because the code that needs to hit this is related to variable sized type accesses. Thanks, Andrew Pinski
Re: get_range_query vs NULL argument
On 2/15/23 14:50, Andrew Pinski wrote:
Hi,
While fixing PR 108354, I came across that
ssa_name_has_boolean_range calls get_range_query with cfun as the
argument but sometimes while in IPA passes cfun is currently nullptr.
The question should we check the argument before calling
get_range_query or is it a valid thing to call it with a nullptr (and
have it return global_ranges in that case)?
That might be ok... personally I see nothing wrong with:
diff --git a/gcc/value-query.h b/gcc/value-query.h
index 63878968118..2d7bf8fcf33 100644
--- a/gcc/value-query.h
+++ b/gcc/value-query.h
@@ -140,7 +140,7 @@ get_global_range_query ()
ATTRIBUTE_RETURNS_NONNULL inline range_query *
get_range_query (const struct function *fun)
{
- return fun->x_range_query ? fun->x_range_query : &global_ranges;
+ return (fun && fun->x_range_query) ? fun->x_range_query : &global_ranges;
}
// Query the global range of NAME in function F. Default to cfun.
The client is probably going to do that anyway.
Aldy?
Re: get_range_query vs NULL argument
On Wed, Feb 15, 2023 at 11:31 PM Andrew MacLeod via Gcc wrote:
>
>
> On 2/15/23 14:50, Andrew Pinski wrote:
> > Hi,
> >While fixing PR 108354, I came across that
> > ssa_name_has_boolean_range calls get_range_query with cfun as the
> > argument but sometimes while in IPA passes cfun is currently nullptr.
> > The question should we check the argument before calling
> > get_range_query or is it a valid thing to call it with a nullptr (and
> > have it return global_ranges in that case)?
>
> That might be ok... personally I see nothing wrong with:
>
> diff --git a/gcc/value-query.h b/gcc/value-query.h
> index 63878968118..2d7bf8fcf33 100644
> --- a/gcc/value-query.h
> +++ b/gcc/value-query.h
> @@ -140,7 +140,7 @@ get_global_range_query ()
> ATTRIBUTE_RETURNS_NONNULL inline range_query *
> get_range_query (const struct function *fun)
> {
> - return fun->x_range_query ? fun->x_range_query : &global_ranges;
> + return (fun && fun->x_range_query) ? fun->x_range_query : &global_ranges;
> }
>
> // Query the global range of NAME in function F. Default to cfun.
>
>
> The client is probably going to do that anyway.
But if there's no 'fun', what is 'global_ranges' initialized for? Or
is 'global_ranges'
usable in IPA context?
> Aldy?
>
>
