Re: [PATCH 3/4] ivopts: Consider cost_step on different forms during unrolling
On Mon, Aug 10, 2020 at 10:41 PM Kewen.Lin wrote: > > Hi Bin, > > on 2020/8/10 下午8:38, Bin.Cheng wrote: > > On Mon, Aug 10, 2020 at 12:27 PM Kewen.Lin wrote: > >> > >> Hi Bin, > >> > >> Thanks for the review!! > >> > >> on 2020/8/8 下午4:01, Bin.Cheng wrote: > >>> Hi Kewen, > >>> Sorry for the late reply. > >>> The patch's most important change is below cost computation: > >>> > @@ -5890,6 +5973,10 @@ determine_iv_cost (struct ivopts_data *data, > struct iv_cand *cand) > cost_step = add_cost (data->speed, TYPE_MODE (TREE_TYPE (base))); > cost = cost_step + adjust_setup_cost (data, cost_base.cost); > > + /* Consider additional step updates during unrolling. */ > + if (data->consider_reg_offset_for_unroll_p && !cand->reg_offset_p) > +cost += (data->current_loop->estimated_unroll - 1) * cost_step; > >>> This is a bit strange, to me the add instructions are additional > >>> computation caused by unrolling+addressing_mode, rather than a native > >>> part in candidate itself. Specifically, an additional cost is needed > >>> if candidates (without reg_offset_p) are chosen for the address type > >>> group/uses. > >> > >> Good point, ideally it should be one additional cost for each cand set, > >> when we select one cand for one group, we need to check this pair need > >> more (estimated_unroll - 1) step costs, we probably need to care about > >> this during remove/replace etc. IIUC the current IVOPTs cost framework > >> doesn't support this and it could increase the selection complexity and > >> time. I hesitated to do it and put it to cand step cost initially instead. > >> > >> I was thinking those candidates with reg_offset_p should be only used for > >> those reg_offset_p groups in most cases (very limited) meanwhile the others > >> are simply scaled up like before. But indeed this can cover some similar > >> cases like one cand is only used for the compare type group which is for > >> loop closing, then it doesn't need more step costs for unrolling. > >> > >> Do you prefer me to improve the current cost framework? > > No, I don't think it's relevant to the candidate selecting algorithm. > > I was thinking about adjusting cost somehow in > > determine_group_iv_cost_address. Given we don't expose selected > > addressing mode in this function, you may need to do it in > > get_address_cost, either way. > > > > Thanks for your suggestion! > > Sorry, I may miss something, but I still think the additional cost is > per candidate. The justification is that we miss to model the iv > candidate step well in the context of unrolling, the step cost is part > of candidate cost, which is per candidate. > > To initialize it in determine_iv_cost isn't perfect as you pointed out, > ideally we should check any uses of the candidate requires iv update > after each replicated iteration, and take extra step costs into account > if at least one needs, meanwhile scaling up all the computation cost to > reflect unrolling cost nature. I see, it's similar to the auto-increment case where cost should be recorded only once. So this is okay given 1) fine predicting rtl-unroll is likely impossible here; 2) the patch has very limited impact. Thanks, bin > > Besides, the reg_offset desirable pair already takes zero cost for > cand/group cost, IIRC negative cost isn't preferred in IVOPTs, are you > suggesting increasing the cost for non reg_offset pairs? If so and per > pair, the extra cost looks possible to be computed several times > unexpectedly. > > >> > + > /* Prefer the original ivs unless we may gain something by replacing > it. > The reason is to make debugging simpler; so this is not relevant for > artificial ivs created by other optimization passes. */ > > >>> > @@ -3654,6 +3729,14 @@ set_group_iv_cost (struct ivopts_data *data, > return; > } > > + /* Since we priced more on non reg_offset IV cand step cost, we > should scale > + up the appropriate IV group costs. Simply consider USE_COMPARE at > the > + loop exit, FIXME if multiple exits supported or no loop exit > comparisons > + matter. */ > + if (data->consider_reg_offset_for_unroll_p > + && group->vuses[0]->type != USE_COMPARE) > +cost *= (HOST_WIDE_INT) data->current_loop->estimated_unroll; > >>> Not quite follow here, giving "pricing more on on-reg_offset IV cand" > >>> doesn't make much sense to me. Also why generic type uses are not > >>> skipped? We want to model the cost required for address computation, > >>> however, for generic type uses there is no way to save the computation > >>> in "address expression". Once unrolled, the computation is always > >>> there? > >>> > >> > >> The main intention is to scale up the group/cand cost for unrolling since > >> we have scaled up the step costs. The assumption is that the original > > If we adjust cost
Re: [PATCH] New test for PR rtl-optimization/96298.
On Tue, Jul 28, 2020 at 10:13:31PM +0100, Roger Sayle wrote:
[ You attached something binary, which makes it hard to reply to. ]
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr96298.c
> @@ -0,0 +1,19 @@
> +/* PR rtl-optimization/96298 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fno-tree-forwprop" } */
> +/* { dg-additional-options "-mno-sse" { target x86_64-*-* i?86-*-* } } */
I would do a test with these target flags in gcc.target/i386/, and one
without any in gcc.dg?
It looks fine to me either way, I think you can use vector_size like
this on any target.
Segher
> +typedef unsigned char __attribute__ ((__vector_size__ (8))) v64u8;
> +
> +v64u8 a;
> +
> +int
> +main (void)
> +{
> + v64u8 x = (a - 1) ^ -a;
> + for (unsigned i = 0; i < sizeof (x); i++)
> +if (x[i] != 0xff)
> + __builtin_abort ();
> + return 0;
> +}
Re: [PATCH] middle-end: Simplify (sign_extend:HI (truncate:QI (ashiftrt:HI X 8)))
Hi!
On Sun, Jul 19, 2020 at 10:42:16AM +0100, Roger Sayle wrote:
> This patch to simplify-rtx.c
> simplifies (sign_extend:HI (truncate:QI (?shiftrt:HI x 8))) to just
> (ashiftrt:HI x 8), as the inner shift already sets the high bits
> appropriately.
> The one oddity of the patch is that it tests for
> LSHIFTRT as inner shift, as simplify/combine has already canonicalized
> this to a logical shift, assuming that the distinction is unimportant
> following the truncation.
If simplify-rtx does that, that is a bug. But combine will do this, I
think that is what you are seeing? Can you verify it already works if
the ASHIFTRT is not changed to an LSHIFTRT?
> + if (is_a (mode, _mode)
> + && is_a (GET_MODE (op), _mode)
> + && is_a (GET_MODE (old_shift), _mode)
> + && GET_MODE_PRECISION (o_mode) - GET_MODE_PRECISION (n_mode)
> + == INTVAL (XEXP (old_shift, 1)))
> + {
> + rtx new_shift = simplify_gen_binary (ASHIFTRT,
> +GET_MODE (old_shift),
> +XEXP (old_shift, 0),
> +XEXP (old_shift, 1));
> + if (GET_MODE_PRECISION (m_mode) > GET_MODE_PRECISION (o_mode))
> + return simplify_gen_unary (SIGN_EXTEND, mode, new_shift,
> +GET_MODE (new_shift));
> + if (mode != GET_MODE (new_shift))
> + return simplify_gen_unary (TRUNCATE, mode, new_shift,
> +GET_MODE (new_shift));
> + return new_shift;
> + }
Yeah looks like it :-)
You could say combine should be smarter about this, but this is a valid
simplification in itself. So, okay for trunk. Thank you!
Segher
Re: [PATCH] middle-end: Fix PR middle-end/85811: Introduce tree_expr_maybe_nan_p et al.
Hi! On Sat, Aug 15, 2020 at 12:10:42PM +0100, Roger Sayle wrote: > I'll quote Joseph Myers (many thanks) who describes things clearly as: > > (a) When both arguments are NaNs, the return value should be a qNaN, > > but sometimes it is an sNaN if at least one argument is an sNaN. Where is this defined? I can't find it in C11, in 18661, and of course it isn't what GCC does (it requires -fsignaling to even acknowledge the existence of signaling NaNs :-) ) > > (b) Under TS 18661-1 semantics, if either argument is an sNaN then the > > result should be a qNaN (whereas if one argument is a qNaN and the > > other is not a NaN, the result should be the non-NaN argument). I cannot find that first part. > if (tree_expr_maybe_signaling_nan_p (arg0) || > tree_expr_maybe_signaling_nan_p (arg1)) > return RECURSE (arg0) && RECURSE (arg1); This new function returns false if !HONOR_SNANS, so this looks good :-) > +bool > +tree_expr_maybe_signaling_nan_p (const_tree x) > +case MIN_EXPR: > +case MAX_EXPR: > + return tree_expr_maybe_signaling_nan_p (TREE_OPERAND (x, 0)) > + || tree_expr_maybe_signaling_nan_p (TREE_OPERAND (x, 1)); Can those ever return a SNaN? What does GCC do for FP_SNANS_ALWAYS_SIGNAL? All looks good to me except the SNaN stuff (which may be just me not understanding it). I find "maybe_" stuff very hard to read and understand btw, but there may be no escaping that :-/ Thanks, Segher
[PATCH] [FIX] Remove object adjustment to preserve object attributes
for bitfield MEMREFs [FIX] Propagate uncached type attributes to unaligned/packed types [ARC] Update tests gcc/ -xx-xx Petro Karashchenko * emit-rtl.c (adjust_address_1): Do not drop the object if the new memory reference is outside the underlying object to preserve object attributes that are needed by some backend implementations. Remove adjust_object parameter as it is not used anymore. (adjust_automodify_address_1): Adjust according to new adjust_address_1 prototype. (replace_equiv_address_nv): Likewise. * gcc/emit-rtl.h (adjust_address): Adjust according to new adjust_address_1 prototype. (adjust_address_nv): Likewise. (adjust_bitfield_address): Likewise. (adjust_bitfield_address_size): Likewise. (adjust_bitfield_address_nv): Likewise. testsuite/ -xx-xx Petro Karashchenko * gcc.target/arc/uncached-9.c: New file. Problem description: __attribute__((uncached)) is dropped for other than the first member of "packed" or "unaligned" types. Tests: Tested with ARC600 bases ASIC. The correct code is generated. 0001-FIX-Remove-object-adjustment-to-preserve-object-attr.patch Description: Binary data
Re: [PATCH] introduce attribute exalias
On 8/14/20 10:43 PM, Alexandre Oliva wrote:
On Aug 14, 2020, Nathan Sidwell wrote:
Since you don't seem to have liked 'aka' either, how about 'nickname',
or 'nicknamed'? A more convenient name to refer to an entity is exactly
what this is about, eh?
I'm sorry, I think those are awful names. They convey no intent. C++ already
has at least 2 'nickname' mechanisms:
using bob = ::foo::bar;
auto = ::elsewhere::object;
'alias' is also now a confusing term, because of the concept of object-aliasing.
The existing alias attribute is defined as:
> The @code{alias} attribute causes the declaration to be emitted as an alias
> for another symbol, which must have been previously declared with the same
> type, and for variables, also the same size and alignment. Declaring an alias
> with a different type than the target is undefined and may be diagnosed. As
> an example, the following declarations:
I.e. it is creating a declaration that is aliased to some other symbol (which
has to also be emitted by the same TU due to the usual elf-like object file
semantics). Notice it says nothing about emitting a *symbol*.
The new attribute is emitting a symbol that equates the declaration it is
attached to (i.e. the other way round).
Its intent is to allow code written in another language to refer to this
definition. I imagine you'd commonly use the foreign language's mangling for
the string provided.
If we spell it 'X', consider:
[[gnu::X ("other")]] int i;
Most commonly, the assembly emitted would contain:
.globl other
.equiv other, i
so, perhaps we should spell it 'equiv'? That's using an existing term.
nathan
--
Nathan Sidwell
Re: Fwd: [PATCH V2 3/4] Work around bootstrap failure in Fortran front end.
Hi, the change looks good to me, OK for master. Regards Thomas This arose from work by Sandra on "Unify C and C++ handling of loops and switches" Kind regards, Toon. Forwarded Message Subject: [PATCH V2 3/4] Work around bootstrap failure in Fortran front end. Date: Thu, 13 Aug 2020 10:34:31 -0600 From: Sandra Loosemore To: gcc-patches@gcc.gnu.org Switching the C++ front end to lower loops the same was as the C front end triggered this error when bootstrapping the Fortran front end: /path/to/gcc/fortran/interface.c:3546:12: error: '*new_arg' may be used uninitialized [-Werror=maybe-uninitialized] 3546 | new_arg[i]->next = NULL; | ~^ Work around this by adding an assertion, which seems appropriate for documentation and good coding practices anyway. 2020-08-12 Sandra Loosemore gcc/fortran/ * interface.c (gfc_compare_actual_formal): Add assertion after main processing loop to silence maybe-uninitialized error. --- gcc/fortran/interface.c | 4 1 file changed, 4 insertions(+) diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c index 7985fc7..9fea94c 100644 --- a/gcc/fortran/interface.c +++ b/gcc/fortran/interface.c @@ -3527,6 +3527,10 @@ gfc_compare_actual_formal (gfc_actual_arglist **ap, gfc_formal_arglist *formal, } } + /* We should have handled the cases where the formal arglist is null + already. */ + gcc_assert (n > 0); + /* The argument lists are compatible. We now relink a new actual argument list with null arguments in the right places. The head of the list remains the head. */
Re: [PATCH] introduce attribute exalias
HI Alexandre I don’t want to derail the discussion - but FIO mostly…. Alexandre Oliva wrote: On Aug 15, 2020, Iain Sandoe wrote: * if the target ABI does not support symbol aliases, then this facility cannot be used. True. I'm surprised there are modern platforms that don’t. different platforms have different designs - it’s not an “old c.f new” thing - see below. What is it that stands in the way? Lack of support for .set in the assembler? If that's the case, couldn't it possibly be worked around by setting multiple global labels at the same spot? I'm pretty sure setting multiple labels at the same address is used and relied on quite often. That’s what’s currently disallowed (the assemblers all support .set). Long ago (before my time with GCC) Darwin’s toolchains did support aliases. The withdrawal was not an accident, but a design choice - where a linker model based on “atoms” was chosen (which requires [as things stand] public symbols to have distinct addresses). I can point you at a description of the linker optimisation if you’re interested. IMO, the atom model can be modified to allow aliases (it might be even that the linker constraint has been relaxed already). However, it’s not my call - I’ve suggested to the platform toolchain team it’s a good idea, but it doesn’t seem to block any other toolchain than GCC so not sure what priority would be assigned. For function aliases, I think there’s a simple work-around and it’s just a question of time for me to make a patch etc. for general aliases to public symbols including data, not so easy. will exclude the GCC targets without symbol aliases from Ada. It's not so dire. Developers for alias-deprived systems would have to use the mangled names instead. That would be a little painful, but not even close to making the language unavailable. Well the predicate was that the use of the mechanism was mandatory, if the existing scheme continues of course there’s no issue. * The process shifts the onus on representation to the exporter and thus there can now be 3 library vendors who all thought “MY_FOO_FUNC” was the best representation for an export - these will now clash in the “shorthand” namespace, although their C++ mangling might well not. Using this to disqualify the new feature would also disqualify regular aliases, that could be used for just the same purpose of making symbols available under chosen names: It wasn’t a comment against the feature - but a comment about shifting the onus for export information onto the producers (and the fact that one can’t generally control what they choose to provide in the absence of a specification - which itanium mangling is). * what happens for templates and overloads - presumably the Ada import has add the relevant (albeit abbreviated) decorations? They don't matter to the proposed design. The reason they come up for you is that you have a completely different solution in mind that requires this kind of resolution. The one I'm proposing attaches the extra aliases directly to the target language entity, be it one of the overloads of a member function, be it a specialization of a template function. Actually, I was thinking about folks who like template metaprogramming (not personally a fan) - and how they would arrange to get automatic export information to track that meta-progamming. Solved if one were able to import the interface…. —— are there other possibilites to solve the underlying issue? C++ mangled names have some proven good properties: * they convey all the relevant information about the interface * they are standardized, and work between implementations from different ‘vendors’ or OSS compilers on the same platform. * they are not going to clash. * they require so much symbolic information that in order to perform mangling you pretty much have to #include all of the relevant C++ headers. Consider typedefs, templates with partial or explicit specializations, default template arguments, besides the possibility of varying definitions across platforms. what about annotating the import pragma in some way such that the platform mangling is applied by the compiler? That would indeed be desirable, but it is unfortunately not viable. I see. Thinking aloud - not thought through in any detail - I wonder if the facilities of C++20 modules are sufficient? *** right now Darwin fails silently (there doesn’t seem to be the usual error that the target doesn’t support that kind of alias). Hmm, thanks, I will make sure there's some more verbose failure mode if we can't find a way for something akin to an alias to be usable there. I imagine it will be easy to fix a diagnostic output. Iain
Re: [PATCH] introduce attribute exalias
On Aug 15, 2020, Iain Sandoe wrote: > what about annotating the import pragma in some way such that the platform > mangling is applied by the compiler? Oh, one more thing about this. Requiring all names to be given in canonical form might alleviate some of the problems I raised, since it would eliminate typedefs and using declarations and directives from consideration. We'd still have other unsurmountable problems to deal with, but more importantly, you wouldn't be able to use u64 any more, you'd have to resolve it to the type that u64 is mapped to, at which point you'd be bringing back the very variation across targets that this feature was designed to overcome. -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer
Re: [PATCH] introduce attribute exalias
On Aug 15, 2020, Iain Sandoe wrote:
> * if the target ABI does not support symbol aliases, then this facility
> cannot
>be used.
True. I'm surprised there are modern platforms that don't.
What is it that stands in the way? Lack of support for .set in the
assembler? If that's the case, couldn't it possibly be worked around by
setting multiple global labels at the same spot? I'm pretty sure
setting multiple labels at the same address is used and relied on quite
often.
>will exclude the GCC targets without symbol aliases from Ada.
It's not so dire. Developers for alias-deprived systems would have to
use the mangled names instead. That would be a little painful, but not
even close to making the language unavailable.
> * The process shifts the onus on representation to the exporter and thus
> there
>can now be 3 library vendors who all thought “MY_FOO_FUNC” was the
>best representation for an export - these will now clash in the “shorthand”
>namespace, although their C++ mangling might well not.
Using this to disqualify the new feature would also disqualify regular
aliases, that could be used for just the same purpose of making symbols
available under chosen names:
extern "C" typeof(foo::func)
__attribute__((__alias__("")))
MY_FOO_FUNC;
Now, this concern appears to be focused on binary-only libraries. Since
we haven't seen vendors rush to make their library internals available
under shorter aliases, polluting the symbolic namespace, I see little
reason for concern about this possibility.
When it comes to binary-only libraries, the ABI is often set in stone,
and it's up to users to figure out the symbol names in the ABI and use
them. If they vary across target platforms, that's inconvenient, but
nothing new.
I expect this feature to be used and useful within multi-language
projects, particularly when using, as part of their interfaces,
shorthand typedefs whose encoding varies depending on the platform.
E.g., consider a function or a template instantiation that takes a
int64_t parameter. Depending on whether int64_t maps to long or long
long, you get different encodings, thus references using the symbol name
have to be adjusted depending on what type stdint.h maps int64_t to.
> * it’s not universally usable without “rebuilding the world” and having
> access to
>source for everything you might want to import
You mean it does not bring improvements to situations in which you can't
introduce nicknames for third-party symbols. You then figure out and
import the mangled names and move on.
> * what happens for templates and overloads - presumably the Ada import has
>add the relevant (albeit abbreviated) decorations?
They don't matter to the proposed design. The reason they come up for
you is that you have a completely different solution in mind that
requires this kind of resolution. The one I'm proposing attaches the
extra aliases directly to the target language entity, be it one of the
overloads of a member function, be it a specialization of a template
function.
> * One can’t have an arbitrary re-name; it has to be supported by the target
>assembler (not that this is a new constraint, but it prevents the exported
>name from being an exact representation of the human-readable C++ interface
>in general).
*nod*. It couldn't be a target for alias attributes otherwise, and I
found that a desirable property to make the new feature useful even for
standalone C++.
> —— are there other possibilites to solve the underlying issue?
> C++ mangled names have some proven good properties:
> * they convey all the relevant information about the interface
> * they are standardized, and work between implementations from different
> ‘vendors’ or OSS compilers on the same platform.
> * they are not going to clash.
* they require so much symbolic information that in order to perform
mangling you pretty much have to #include all of the relevant C++
headers.
Consider typedefs, templates with partial or explicit specializations,
default template arguments, besides the possibility of varying
definitions across platforms.
> what about annotating the import pragma in some way such that the platform
> mangling is applied by the compiler?
That would indeed be desirable, but it is unfortunately not viable.
Consider you have to figure out the correct mangling for this:
foo::bar::f
Is foo a namespace, a canonical class name, or a typedef? (maybe even a
using declaration, or even something brought into the global namespace
by a using directive)
If it's a typedef, what's the canonical name? (it could be a template
instantiation)
bar is clearly a template type, so you "just" need enough symbolic
information to be able to mangle std::iostream&, std::string, u64,
g::h.
For u64, you just have to look at stdint.h to see which of the
C++-defined types is maps to.
g::h is a mystery. It could be a type, a function, a member function, a
variable, a
Re: [PATCH] improve memcmp and memchr constant folding (PR 78257)
Hi Martin,
On Sat, 15 Aug 2020 at 01:14, Martin Sebor via Gcc-patches
wrote:
>
> On 8/13/20 11:44 AM, Martin Sebor wrote:
> > On 8/13/20 10:21 AM, Jeff Law wrote:
> >> On Fri, 2020-07-31 at 17:55 -0600, Martin Sebor via Gcc-patches wrote:
> >>> The folders for these functions (and some others) call c_getsr
> >>> which relies on string_constant to return the representation of
> >>> constant strings. Because the function doesn't handle constants
> >>> of other types, including aggregates, memcmp or memchr calls
> >>> involving those are not folded when they could be.
> >>>
> >>> The attached patch extends the algorithm used by string_constant
> >>> to also handle constant aggregates involving elements or members
> >>> of the same types as native_encode_expr. (The change restores
> >>> the empty initializer optimization inadvertently disabled in
> >>> the fix for pr96058.)
> >>>
> >>> To avoid accidentally misusing either string_constant or c_getstr
> >>> with non-strings I have introduced a pair of new functions to get
> >>> the representation of those: byte_representation and getbyterep.
> >>>
> >>> Tested on x86_64-linux.
> >>>
> >>> Martin
> >>
> >>> PR tree-optimization/78257 - missing memcmp optimization with
> >>> constant arrays
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> PR middle-end/78257
> >>> * builtins.c (expand_builtin_memory_copy_args): Rename called
> >>> function.
> >>> (expand_builtin_stpcpy_1): Remove argument from call.
> >>> (expand_builtin_memcmp): Rename called function.
> >>> (inline_expand_builtin_bytecmp): Same.
> >>> * expr.c (convert_to_bytes): New function.
> >>> (constant_byte_string): New function (formerly string_constant).
> >>> (string_constant): Call constant_byte_string.
> >>> (byte_representation): New function.
> >>> * expr.h (byte_representation): Declare.
> >>> * fold-const-call.c (fold_const_call): Rename called function.
> >>> * fold-const.c (c_getstr): Remove an argument.
> >>> (getbyterep): Define a new function.
> >>> * fold-const.h (c_getstr): Remove an argument.
> >>> (getbyterep): Declare a new function.
> >>> * gimple-fold.c (gimple_fold_builtin_memory_op): Rename callee.
> >>> (gimple_fold_builtin_string_compare): Same.
> >>> (gimple_fold_builtin_memchr): Same.
> >>>
> >>> gcc/testsuite/ChangeLog:
> >>>
> >>> PR middle-end/78257
> >>> * gcc.dg/memchr.c: New test.
> >>> * gcc.dg/memcmp-2.c: New test.
> >>> * gcc.dg/memcmp-3.c: New test.
> >>> * gcc.dg/memcmp-4.c: New test.
> >>>
> >>> diff --git a/gcc/expr.c b/gcc/expr.c
> >>> index a150fa0d3b5..a124df54655 100644
> >>> --- a/gcc/expr.c
> >>> +++ b/gcc/expr.c
> >>> @@ -11594,15 +11594,103 @@ is_aligning_offset (const_tree offset,
> >>> const_tree exp)
> >>> /* This must now be the address of EXP. */
> >>> return TREE_CODE (offset) == ADDR_EXPR && TREE_OPERAND (offset,
> >>> 0) == exp;
> >>> }
> >>> -
> >>> -/* Return the tree node if an ARG corresponds to a string constant
> >>> or zero
> >>> - if it doesn't. If we return nonzero, set *PTR_OFFSET to the
> >>> (possibly
> >>> - non-constant) offset in bytes within the string that ARG is
> >>> accessing.
> >>> - If MEM_SIZE is non-zero the storage size of the memory is returned.
> >>> - If DECL is non-zero the constant declaration is returned if
> >>> available. */
> >>> -tree
> >>> -string_constant (tree arg, tree *ptr_offset, tree *mem_size, tree
> >>> *decl)
> >>> +/* If EXPR is a constant initializer (either an expression or
> >>> CONSTRUCTOR),
> >>> + attempt to obtain its native representation as an array of
> >>> nonzero BYTES.
> >>> + Return true on success and false on failure (the latter without
> >>> modifying
> >>> + BYTES). */
> >>> +
> >>> +static bool
> >>> +convert_to_bytes (tree type, tree expr, vec *bytes)
> >>> +{
> >>> + if (TREE_CODE (expr) == CONSTRUCTOR)
> >>> +{
> >>> + /* Set to the size of the CONSTRUCTOR elements. */
> >>> + unsigned HOST_WIDE_INT ctor_size = bytes->length ();
> >>> +
> >>> + if (TREE_CODE (type) == ARRAY_TYPE)
> >>> +{
> >>> + tree val, idx;
> >>> + tree eltype = TREE_TYPE (type);
> >>> + unsigned HOST_WIDE_INT elsize =
> >>> +tree_to_uhwi (TYPE_SIZE_UNIT (eltype));
> >>> + unsigned HOST_WIDE_INT i, last_idx = HOST_WIDE_INT_M1U;
> >>> + FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (expr), i, idx, val)
> >>> +{
> >>> + /* Append zeros for elements with no initializers. */
> >>> + if (!tree_fits_uhwi_p (idx))
> >>> +return false;
> >>> + unsigned HOST_WIDE_INT cur_idx = tree_to_uhwi (idx);
> >>> + if (unsigned HOST_WIDE_INT size = cur_idx - (last_idx + 1))
> >>> +{
> >>> + size = size * elsize + bytes->length ();
> >>> + bytes->safe_grow_cleared (size);
> >^^^
> >
> >>> +}
> >>> +
> >>> + if
Re: [committed] analyzer: fix initialization from constant pool [PR96609, PR96616]
On Sat, 15 Aug 2020 at 00:52, David Malcolm wrote:
>
> PR testsuite/96609 and PR analyzer/96616 report various testsuite
> failures seen on powerpc64, aarch64, and arm in new tests added by
> r11-2694-g808f4dfeb3a95f50f15e71148e5c1067f90a126d.
>
> Some of these failures (in gcc.dg/analyzer/init.c, and on arm
> in gcc.dg/analyzer/casts-1.c) relate to initializations from var_decls
> in the constant pool. I wrote the tests assuming that the gimplified
> stmts would initialize the locals via a gassign of code CONSTRUCTOR,
> whereas on these targets some of the initializations are gassign from
> a VAR_DECL e.g.:
> c = *.LC0;
> where "*.LC0" is a var_decl with DECL_IN_CONSTANT_POOL set.
>
> For example, in test_7:
>struct coord c[2] = {{3, 4}, {5, 6}};
>__analyzer_eval (c[0].x == 3); /* { dg-warning "TRUE" } */
> after the initialization, the store was simply recording:
>cluster for: c: INIT_VAL(*.LC0)
> when I was expecting the cluster for c to have:
> cluster for: c
> key: {kind: direct, start: 0, size: 32, next: 32}
> value: 'int' {(int)3}
> key: {kind: direct, start: 32, size: 32, next: 64}
> value: 'int' {(int)4}
> key: {kind: direct, start: 64, size: 32, next: 96}
> value: 'int' {(int)5}
> key: {kind: direct, start: 96, size: 32, next: 128}
> value: 'int' {(int)6}
> The test for c[0].x == 3 would then generate:
> cluster for: _2: (SUB(SUB(INIT_VAL(*.LC0), c[(int)0]), c[(int)0].x)==(int)3)
> which is UNKNOWN, leading to the test failing.
>
> This patch fixes the init.c and casts-1.c failures by special-casing
> reads from a var_decl with DECL_IN_CONSTANT_POOL set, so that they build
> a compound_svalue containing the bindings implied by the CONSTRUCTOR
> node for DECL_INITIAL.
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> Manually verified the fixes to init.c and casts-1.c on
> aarch64-unknown-linux-gnu, arm-unknown-eabi, and powerpc64-linux-gnu
> (-m32 and -m64).
>
> Pushed to master as r11-2708-g2867118ddda9b56d991c16022f7d3d634ed08313.
>
Hi David,
Thanks for fixing this.
However, this patch is causing 2 ICEs on arm:
gcc.dg/analyzer/data-model-1.c (internal compiler error)
gcc.dg/analyzer/pr94639.c (internal compiler error)
Christophe
> This doesn't address the bogus -Wanalyzer-too-complex messages
> for pr93032-mztools.c reported in the bugs, which seem to be a
> separate issue that I'm now investigating.
>
> gcc/analyzer/ChangeLog:
> PR testsuite/96609
> PR analyzer/96616
> * region-model.cc (region_model::get_store_value): Call
> maybe_get_constant_value on decl_regions first.
> * region-model.h (decl_region::maybe_get_constant_value): New decl.
> * region.cc (decl_region::get_stack_depth): Likewise.
> (decl_region::maybe_get_constant_value): New.
> * store.cc (get_subregion_within_ctor): New.
> (binding_map::apply_ctor_to_region): New.
> * store.h (binding_map::apply_ctor_to_region): New decl.
> ---
> gcc/analyzer/region-model.cc | 5 +++
> gcc/analyzer/region-model.h | 2 ++
> gcc/analyzer/region.cc | 27 +
> gcc/analyzer/store.cc| 59
> gcc/analyzer/store.h | 3 ++
> 5 files changed, 96 insertions(+)
>
> diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
> index 649e20438e4..3c7ea40e8d8 100644
> --- a/gcc/analyzer/region-model.cc
> +++ b/gcc/analyzer/region-model.cc
> @@ -1192,6 +1192,11 @@ region_model::get_rvalue (tree expr,
> region_model_context *ctxt)
> const svalue *
> region_model::get_store_value (const region *reg) const
> {
> + /* Special-case: handle var_decls in the constant pool. */
> + if (const decl_region *decl_reg = reg->dyn_cast_decl_region ())
> +if (const svalue *sval = decl_reg->maybe_get_constant_value (m_mgr))
> + return sval;
> +
>const svalue *sval
> = m_store.get_any_binding (m_mgr->get_store_manager (), reg);
>if (sval)
> diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
> index 33aa3461611..3d044bf8d6c 100644
> --- a/gcc/analyzer/region-model.h
> +++ b/gcc/analyzer/region-model.h
> @@ -1869,6 +1869,8 @@ public:
>tree get_decl () const { return m_decl; }
>int get_stack_depth () const;
>
> + const svalue *maybe_get_constant_value (region_model_manager *mgr) const;
> +
> private:
>tree m_decl;
> };
> diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc
> index f3f577c43de..afe416b001b 100644
> --- a/gcc/analyzer/region.cc
> +++ b/gcc/analyzer/region.cc
> @@ -874,6 +874,33 @@ decl_region::get_stack_depth () const
>return 0;
> }
>
> +/* If the underlying decl is in the global constant pool,
> + return an svalue representing the constant value.
> + Otherwise return NULL. */
> +
> +const svalue *
> +decl_region::maybe_get_constant_value (region_model_manager *mgr) const
> +{
> + if (TREE_CODE (m_decl) == VAR_DECL
> +
Re: [PATCH] middle-end: Recognize idioms for bswap32 and bswap64 in match.pd.
On Sat, Aug 15, 2020 at 11:09:17AM +0100, Roger Sayle wrote: > +/* Recognize ((T)bswap32(x)<<32)|bswap32(x>>32) as bswap64(x). */ > +(simplify > + (bit_ior:c Any reason for supporting bit_ior only? Don't plus:c or bit_xor:c work the same (i.e. use (for op (bit_ior bit_xor plus) ...)? Jakub
PINGs
The following patches are still awaiting review (longer than a week or two). gfortran: Improve translation of POPPAR intrinsic https://gcc.gnu.org/pipermail/gcc-patches/2020-June/548055.html PR middle-end/90597: gcc_assert ICE in layout_type https://gcc.gnu.org/pipermail/gcc-patches/2020-June/549128.html middle-end: Simplify (sign_extend:HI (truncate:QI (ashiftrt:HI X 8))) https://gcc.gnu.org/pipermail/gcc-patches/2020-July/550258.html New test for PR rtl-optimization/96298. https://gcc.gnu.org/pipermail/gcc-patches/2020-July/550911.html middle-end: Recognize/canonicalize MULT_HIGHPART_EXPR and expand it. https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551316.html Many thanks in advance. Roger --
[PATCH] middle-end: Fix PR middle-end/85811: Introduce tree_expr_maybe_nan_p et al.
The motivation for this patch is PR middle-end/85811, a wrong-code regression entitled "Invalid optimization with fmax, fabs and nan". The optimization involves assuming max(x,y) is non-negative if (say) y is non-negative, i.e. max(x,2.0). Unfortunately, this is an invalid assumption in the presence of NaNs. Hence max(x,+qNaN), with IEEE fmax semantics will always return x even though the qNaN is non-negative. Worse, max(x,2.0) may return a negative value if x is -sNaN. I'll quote Joseph Myers (many thanks) who describes things clearly as: > (a) When both arguments are NaNs, the return value should be a qNaN, > but sometimes it is an sNaN if at least one argument is an sNaN. > (b) Under TS 18661-1 semantics, if either argument is an sNaN then the > result should be a qNaN (whereas if one argument is a qNaN and the > other is not a NaN, the result should be the non-NaN argument). > Various implementations treat sNaNs like qNaNs here. Under this logic, the tree_expr_nonnegative_p for IEEE fmax should be: CASE_CFN_FMAX: CASE_CFN_FMAX_FN: /* Usually RECURSE (arg0) || RECURSE (arg1) but NaNs complicate things. In the presence of sNaNs, we're only guaranteed to be non-negative if both operands are non-negative. In the presence of qNaNs, we're non-negative if either operand is non-negative and can't be a qNaN, or if both operands are non-negative. */ if (tree_expr_maybe_signaling_nan_p (arg0) || tree_expr_maybe_signaling_nan_p (arg1)) return RECURSE (arg0) && RECURSE (arg1); return RECURSE (arg0) ? (!tree_expr_maybe_nan_p (arg0) || RECURSE (arg1)) : (RECURSE (arg1) && !tree_expr_maybe_nan_p (arg1)); Which indeed resolves the wrong code in the PR. The infrastructure that makes this possible are the two new functions tree_expr_maybe_nan_p and tree_expr_maybe_signaling_nan_p which test whether a value may potentially be a NaN or a signaling NaN respectively. In fact, this patch adds seven new predicates to the middle-end: bool tree_expr_finite_p (const_tree); bool tree_expr_infinite_p (const_tree); bool tree_expr_maybe_infinite_p (const_tree); bool tree_expr_signaling_nan_p (const_tree); bool tree_expr_maybe_signaling_nan_p (const_tree); bool tree_expr_nan_p (const_tree); bool tree_expr_maybe_nan_p (const_tree); These functions correspond to the "must" and "may" operators in modal logic, and allow us to triage expressions in the middle-end; definitely a NaN, definitely not a NaN, and unknown at compile-time, etc. A prime example of the utility of these functions is that a IEEE floating point value promoted from an integer type can't be a NaN or infinite. Hence (double)i+0.0 where i is an integer can be simplified to (double)i even with -fsignaling-nans. Currently in GCC optimizations are enabled/disabled based on whether the expression's type supports NaNs or sNaNs; with these new predicates they can be controlled by whether the actual operands may or may not be NaNs. Having added these extremely useful helper functions to the middle-end, I couldn't help by use then in a few places in fold-const.c, builtins.c and match.pd. In the near term, these can/should be used in places where the tree optimizers test for HONOR_NANS, HONOR_INFINITIES or HONOR_SNANS, or explicitly test whether a REAL_CST is a NaN or Inf. In the longer term (I'm not volunteering) these predicates could perhaps be hooked into the middle-end's SSA chaining and/or VRP machinery, allowing finiteness to propagated around the CFG, much like we currently propagate value ranges. This patch has been tested on x86_64-pc-linux-gnu with a "make bootstrap" and "make -k check". Ok for mainline? 2020-08-15 Roger Sayle gcc/ChangeLog PR middle-end/85811 * fold-const.c (tree_expr_finite_p): New function to test whether a tree expression must be finite, i.e. not a FP NaN or infinity. (tree_expr_infinite_p): New function to test whether a tree expression must be infinite, i.e. a FP infinity. (tree_expr_maybe_infinite_p): New function to test whether a tree expression may be infinite, i.e. a FP infinity. (tree_expr_signaling_nan_p): New function to test whether a tree expression must evaluate to a signaling NaN (sNaN). (tree_expr_maybe_signaling_nan_p): New function to test whether a tree expression may be a signaling NaN (sNaN). (tree_expr_nan_p): New function to test whether a tree expression must evaluate to a (quiet or signaling) NaN. (tree_expr_maybe_nan_p): New function to test whether a tree expression me be a (quiet or signaling) NaN. (tree_binary_nonnegative_warnv_p) [MAX_EXPR]: In the presence of NaNs, MAX_EXPR is only guaranteed to be non-negative, if both operands are non-negative. (tree_call_nonnegative_warnv_p)
RE: [PATCH] middle-end: Recognize idioms for bswap32 and bswap64 in match.pd.
Hi Marc,
Here's version #2 of the patch to recognize bswap32 and bswap64
incorporating your
suggestions and feedback. The test cases now confirm the transformation is
applied
when int is 32 bits and long is 64 bits, and should pass otherwise; the
patterns now
reuse (more) capturing groups, and the patterns have been made more generic
to allow
the ultimate type to be signed or unsigned (hence there are now two new
gcc.dg tests).
Alas my efforts to allow the input argument to be signed, and use
fold_convert to coerce
it to the correct type before calling __builtin_bswap failed, with the error
messages:
>fold-bswap-2.c: In function 'swap64':
>fold-bswap-2.c:22:1: error: invalid argument to gimple call
>(long unsigned int) x_6(D)
>_12 = __builtin_bswap64 ((long unsigned int) x_6(D));
>during GIMPLE pass: forwprop
>fold-bswap-2.c:22:1: internal compiler error: verify_gimple failed
So I require arguments to be the expected type for now. If anyone's
sufficiently motivated
to support these cases, this can be done as a follow-up patch.
This revised patch has been tested on x86_64-pc-linux-gnu with a "make
bootstrap"
and "make -k check" with no new failures.
Ok for mainline?
Thanks in advance,
Roger
--
-Original Message-
From: Marc Glisse
Sent: 12 August 2020 10:43
To: Roger Sayle
Cc: 'GCC Patches'
Subject: Re: [PATCH] middle-end: Recognize idioms for bswap32 and bswap64 in
match.pd.
On Wed, 12 Aug 2020, Roger Sayle wrote:
> This patch is inspired by a small code fragment in comment #3 of
> bugzilla PR rtl-optimization/94804. That snippet appears almost
> unrelated to the topic of the PR, but recognizing __builtin_bswap64
> from two __builtin_bswap32 calls, seems like a clever/useful trick.
> GCC's optabs.c contains the inverse logic to expand bswap64 by IORing
> two bswap32 calls, so this transformation/canonicalization is safe,
> even on targets without suitable optab support. But on x86_64, the
> swap64 of the test case becomes a single instruction.
>
>
> This patch has been tested on x86_64-pc-linux-gnu with a "make
> bootstrap" and a "make -k check" with no new failures.
> Ok for mainline?
Your tests seem to assume that int has 32 bits and long 64.
+ (if (operand_equal_p (@0, @2, 0)
Why not reuse @0 instead of introducing @2 in the pattern? Similarly, it may
be a bit shorter to reuse @1 instead of a new @3 (I don't think the tricks
with @@ will be needed here).
+ && types_match (TREE_TYPE (@0), uint64_type_node)
that seems very specific. What goes wrong with a signed type for instance?
+(simplify
+ (bit_ior:c
+(lshift
+ (convert (BUILT_IN_BSWAP16 (convert (bit_and @0
+ INTEGER_CST@1
+ (INTEGER_CST@2))
+(convert (BUILT_IN_BSWAP16 (convert (rshift @3
+ INTEGER_CST@4)
I didn't realize we kept this useless bit_and when casting to a smaller
type. We probably get a different pattern on 16-bit targets, but a pattern
they do not match won't hurt them.
--
Marc Glisse
diff --git a/gcc/match.pd b/gcc/match.pd
index c3b8816..c682d3d 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -3410,6 +3410,33 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
(bswap (bitop:c (bswap @0) @1))
(bitop @0 (bswap @1)
+/* Recognize ((T)bswap32(x)<<32)|bswap32(x>>32) as bswap64(x). */
+(simplify
+ (bit_ior:c
+(lshift (convert (BUILT_IN_BSWAP32 (convert@0 @1)))
+ INTEGER_CST@2)
+(convert (BUILT_IN_BSWAP32 (convert@3 (rshift @1 @2)
+ (if (INTEGRAL_TYPE_P (type)
+ && TYPE_PRECISION (type) == 64
+ && types_match (TREE_TYPE (@1), uint64_type_node)
+ && types_match (TREE_TYPE (@0), uint32_type_node)
+ && types_match (TREE_TYPE (@3), uint32_type_node)
+ && wi::to_widest (@2) == 32)
+(convert (BUILT_IN_BSWAP64 @1
+
+/* Recognize ((T)bswap16(x)<<16)|bswap16(x>>16) as bswap32(x). */
+(simplify
+ (bit_ior:c
+(lshift
+ (convert (BUILT_IN_BSWAP16 (convert (bit_and @0 INTEGER_CST@1
+ (INTEGER_CST@2))
+(convert (BUILT_IN_BSWAP16 (convert (rshift @0 @2)
+ (if (INTEGRAL_TYPE_P (type)
+ && TYPE_PRECISION (type) == 32
+ && types_match (TREE_TYPE (@0), uint32_type_node)
+ && wi::to_widest (@1) == 65535
+ && wi::to_widest (@2) == 16)
+(convert (BUILT_IN_BSWAP32 @0
/* Combine COND_EXPRs and VEC_COND_EXPRs. */
diff --git a/gcc/testsuite/gcc.dg/fold-bswap-1.c
b/gcc/testsuite/gcc.dg/fold-bswap-1.c
new file mode 100644
index 000..3abb862
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fold-bswap-1.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+unsigned int swap32(unsigned int x)
+{
+ if (sizeof(unsigned int)==4 && sizeof(unsigned short)==2) {
+unsigned int a = __builtin_bswap16(x);
+x >>= 16;
+a <<= 16;
+return __builtin_bswap16(x) | a;
+ } else return __builtin_bswap32(x);
+}
+
+unsigned long swap64(unsigned
Re: [PATCH] introduce attribute exalias
Hi Alexandre,
I built the patch on x86_64-linux and darwin*** (fwiw).
* It’s firmly agreed that there are times when referring to C++ mangled names
is less than ideal.
* IIUC, the objective is to have a short-hand way of annotating an export
from
C++ so that it’s (a) more human-readable and (b) independent of any platform
variation in mangling of types - in the Ada import pragma?
I see Nathan commented that there are no language-lawyering implications,
which is good.
However, there do seem to be both ABI and engineering implications:
* if the target ABI does not support symbol aliases, then this facility cannot
be used. Which either means that you cannot rely on this facility (and thus
make things generically easier in the Ada implementation on GCC) or you
will exclude the GCC targets without symbol aliases from Ada. The latter
would make me sad as Darwin maintainer.
* the symbol table will grow (maybe one doesn’t care if there are not many).
* The process shifts the onus on representation to the exporter and thus there
can now be 3 library vendors who all thought “MY_FOO_FUNC” was the
best representation for an export - these will now clash in the “shorthand”
namespace, although their C++ mangling might well not.
* it’s not universally usable without “rebuilding the world” and having access
to
source for everything you might want to import (maybe not important, depends
on whether there are users of Ada who depend on closed source libraries).
* what happens for templates and overloads - presumably the Ada import has
add the relevant (albeit abbreviated) decorations?
* One can’t have an arbitrary re-name; it has to be supported by the target
assembler (not that this is a new constraint, but it prevents the exported
name from being an exact representation of the human-readable C++ interface
in general).
—— are there other possibilites to solve the underlying issue?
C++ mangled names have some proven good properties:
* they convey all the relevant information about the interface
* they are standardized, and work between implementations from different
‘vendors’ or OSS compilers on the same platform.
* they are not going to clash.
The compiler already has a proven implementation of C++ mangling rules.
what about annotating the import pragma in some way such that the platform
mangling is applied by the compiler?
thus, in my example below, so long as the interface is represented
correctly by the
string after “itanium:” it will mangle correctly for the target (even if
that mangling would
alter between targets for representation of some types).
I suppose there’s a question of how many pragmas would be needed to annotate
sufficiently to get the mangling right?
pragma CPP_Constructor (New_Animal);
pragma Import (CPP, New_Animal, itanium:”Animal()");
It seems one ideally wants the Ada moral equivalent of:
extern “C++” {
things we want to import.
}
which automagically does the right interface transforms and synthesizes the
Ada interfaces required.
Alexandre Oliva wrote:
On Aug 14, 2020, Nathan Sidwell wrote:
Perhaps alias is not the right name at all.
I kind of like the explicit present of "alias" because, well, what we
get is an alias, to the point that, if asm aliases aren't available, it
won't work. And, if they are, you can use the so-assigned name as an
alias target, so it's a good thing if they're typographically related.
One could even argue that this new attribute is more deserving of the
term alias than the existing one, and that the existing one should be
renamed to "aliased_to" or so. But I'm not seriously suggesting us to
rename a long-available attribute while assigning uses thereof a
different semantics, that would be preposterous.
Since you don't seem to have liked 'aka' either, how about 'nickname',
or 'nicknamed'? A more convenient name to refer to an entity is exactly
what this is about, eh?
.. assuming this facility was added ...
.. my 0.02GBP contribution the the bikeshed painting fund would be….
it’s nice when an attribute reads in the source to tell you its purpose.
so how about:
“export_as” or “exported_as”
(depending on whether one regards this as a command to the compiler,
or an annotation).
so :
__attribute__ ((__export_as__ ("Ctor_For_Animal"))) // extra alias
Animal() {Age_Count = 0;};
or:
__attribute__ ((__exported_as__ ("Ctor_For_Animal"))) // extra alias
Animal() {Age_Count = 0;};
thanks
Iain
*** right now Darwin fails silently (there doesn’t seem to be the usual error
that the target doesn’t support that kind of alias).
