Re: [Patch, fortran] PR 49010/24518 MOD/MODULO fixes, take 2
On Fri, May 4, 2012 at 6:11 PM, Mikael Morin wrote: > On 02/05/2012 21:22, Janne Blomqvist wrote: >> PING #2 >> >> On Thu, Apr 26, 2012 at 12:20 AM, Janne Blomqvist >> wrote: >>> PING! >>> >>> On Thu, Apr 19, 2012 at 00:46, Janne Blomqvist >>> wrote: Hi, the attached patch implements a few fixes and cleanups for the MOD and MODULO intrinsics. - When the arguments are constant, use mpfr_fmod instead of the naive algorithms which are numerically unstable for large arguments. This extends the PR 24518 fix to constant arguments as well, and makes the compile-time evaluation match the runtime implementation which also uses fmod in the same manner. - Remove the old fallback path for the case builtin_fmod is not available, as the builtin is AFAICS always available. - Specify the behavior wrt. the sign and magnitude of the result, mention this in the documentation. This includes signed zeros which now behave similar to other finite values. I.e. for MOD(A, P) the result has the sign of A and a magnitude less than that of P, and for MODULO(A, P) the result has the sign of P and a magnitude less than that of P. As a (minor?) caveat, at runtime this depends on the implementation of the fmod function in the target C library. But, a fmod that follows C99 Annex F implements this behavior. Regtested on x86_64-unknown-linux-gnu, Ok for trunk? 2012-04-19 Janne Blomqvist PR fortran/49010 PR fortran/24518 * intrinsic.texi (MOD, MODULO): Mention sign and magnitude of result. * simplify.c (gfc_simplify_mod): Use mpfr_fmod. (gfc_simplify_modulo): Likewise, use copysign to fix the result if zero. * trans-intrinsic.c (gfc_conv_intrinsic_mod): Remove fallback as builtin_fmod is always available. For modulo, call copysign to fix the result when signed zeros are enabled. -- Janne Blomqvist >>> >>> >>> >>> -- >>> Janne Blomqvist >> >> >> > > Hello, > > this looks good. OK with a testcase. Committed as r187191. Testcases I committed attached. > Thanks, and sorry for the delay. Thanks for the review! -- Janne Blomqvist mod_large_1.f90 Description: Binary data mod_sign0_1.f90 Description: Binary data
[RFC] PR 53063 encode group options in .opt files
Hi, This patch is a first step towards encoding the fact that some flags enable other flags in the .opt files. As a proof-of-concept, I have only implemented the check for a group option not overriding the value of the suboption if the suboption was set. In the future, we should handle more stuff automatically. However, I think this patch already leads to the removal of a big chunk of code. Bootstrapped and tested. I don't provide changelog and the code has some stylistic issues that I will fix if the strategy is acceptable. About the implementation, in PR53063 I proposed Wall C ObjC C++ ObjC++ Warning Enable most warning messages Enables(Wwhatever, Wfoo=2) However this would make difficult to implement options that are enabled by the combination of two options, like -Wunused -Wextra enables -Wunused-parameter. So I decided for, Wunused-value -Common Var(warn_unused_value) Init(-1) Warning +Common Var(warn_unused_value) Init(-1) Warning EnabledBy(Wunused) This can be easily extended to EnabledBy(Wunused & Wextra) or an arbitrary combination of options and logical operators. Comments? My knowledge of awk is basically zero, so suggestions on how to improve the code are very welcome. Should I cleanup the patch and submit it with a Changelog? Cheers, Manuel. group-options.diff Description: Binary data
PR 43772 Errant -Wlogical-op warning when testing limits
This patch fixes almost all false positives in PR43772. The case not fixed is:
intmax_t i = (whatever);
if (INT_MAX < i && i <= LONG_MAX)
print ("i is in 'long' but not 'int' ran")
where we warn if INT_MAX = LONG_MAX < INTMAX_MAX. Perhaps with the
macro location code, we could now tell that the constants INT_MAX and
LONG_MAX come from different macro expansions in system headers, and
avoid warning in this specific case, but that would be better done in
a follow-up patch. Dodji, is that possible? how could it be done?
Bootstrapped and regression tested.
OK?
2012-05-05 Manuel López-Ibáñez
PR c/43772
c-family/
* c-common.c (warn_logical_operator): Do not warn if either side
is already true or false.
testsuite/
* c-c++-common/pr43772.c: New.
fix-pr43772.diff
Description: Binary data
[RFC] PR 51712 -Wtype-limits should not trigger for enum constants
This patch adds the ability to see in shorten_compare that a constant comes from an enumeral type. It does so by using in both C/C++ the existing infrastructure from the C FE (c_expr). Currently, only the C FE carries down the correct information to avoid warning, so C++ still warns. However, this patch also opens the possibility to pass down this info in the C++ FE using c_expr, and avoid warning in a follow up patch. Bootstrapped and regression tested. This is a proof-of-concept, so I don't provide a changelog or update the comments of new/existing functions. Comments? Is this approach OK? Cheers, Manuel. fix-pr51712-2.diff Description: Binary data
[RFH / Patch] C++/52282
Hi,
I'm analyzing this PR and the various testcases which come with it. It
looks like we have indeed two separate issues: one, which looks simpler,
with decltype, leading to ICEs; a more complex one with constexpr. The
former is about ADDR_EXPR unhandled in the finish_decltype_type switch
for this testcase:
template
struct C
{
static constexpr decltype(V) c() { return V; }
};
struct D
{
static constexpr int d() { return 10; }
};
static_assert((C::c())() == 10, "oops");
Would it make sense to just handle ADDR_EXPR too like in the patchlet
below? It seems we are in this case too in the same situation which led
to handling INTEGER_CST and PTRMEM_CST, or we have an issue with
const-ness?
The problems with constexpr look more nasty, are all about testcases
similar to the above, many variants of it, like:
template
struct B
{
typedef T type;
static constexpr type b() { return V; }
};
struct D
{
static constexpr int d() { return 10; }
};
static_assert((B::b())() == 10, "oops");
which is rejected like:
52282.C:32:1: error: non-constant condition for static assertion
static_assert((B::b())() == 10, "oops"); // line 30
52282.C:32:38: error: expression ‘D::d’ does not designate a constexpr
function
static_assert((B::b())() == 10, "oops"); // line 30
For this kind of testcase I see a lot of NOP_EXPRs around, which I don't
really understand and *appear* to confuse quite a bit code we have in
the cxx_eval_* functions. For example, for the above, one crops up at
the beginning of cxx_eval_call_expression:
if (TREE_CODE (fun) != FUNCTION_DECL)
{
/* Might be a constexpr function pointer. */
fun = cxx_eval_constant_expression (old_call, fun, allow_non_constant,
/*addr*/false, non_constant_p);
if (TREE_CODE (fun) == ADDR_EXPR)
fun = TREE_OPERAND (fun, 0);
}
as the fun returned by cxx_eval_constant_expression and the logic
handling ADDR_EXPR doesn't seem to work as designed. If I force a
STRIP_NOPS the testcase is "accepted". But really I see something
brittle about all these NOP_EXPRs and I'm looking for comments / hints.
I don't think we just want to add STRIP_NOPS in a ton of places, which,
if I understand correctly, would probably also imply the need to
unshare_expr, but I don't have brilliant ideas right now...
Another kind of weird issue we are facing is for:
template
struct W_ { static constexpr T value = V; };
constexpr struct C {
constexpr int c1() const { return 10; }
} c;
static_assert((c.*W_::value)() == 10, "oops");
which leads to:
ice.cpp:72:1: error: non-constant condition for static assertion
static_assert((c.*W_::value)() == 10, "oops");
ice.cpp:72:56: error: expression ‘0u’ does not designate a constexpr
function
static_assert((c.*W_::value)() == 10, "oops");
note the '0u'! (the error message comes from the beginning of
cxx_eval_call_expression) I guess these issues should not require major
surgeries if a testcase like the latter but using, instead of W_:
template
struct W { static constexpr T value() { return V; } };
works fine. In this case too, if I follow the long chain of cxx_eval_* I
see a lot of NOP_EXPRs, but I'm not sure it's all there is to the issue,
maybe we are just doing something wrong with the pointers...
Thanks for any comment!
Paolo.
Index: semantics.c
===
--- semantics.c (revision 187192)
+++ semantics.c (working copy)
@@ -5268,6 +5268,7 @@ finish_decltype_type (tree expr, bool id_expressio
case INTEGER_CST:
case PTRMEM_CST:
+ case ADDR_EXPR:
/* We can get here when the id-expression refers to an
enumerator or non-type template parameter. */
type = TREE_TYPE (expr);
Re: PR 43772 Errant -Wlogical-op warning when testing limits
On Sat, May 5, 2012 at 3:52 AM, Manuel López-Ibáñez
wrote:
> This patch fixes almost all false positives in PR43772. The case not fixed is:
>
> intmax_t i = (whatever);
> if (INT_MAX < i && i <= LONG_MAX)
> print ("i is in 'long' but not 'int' ran")
>
> where we warn if INT_MAX = LONG_MAX < INTMAX_MAX. Perhaps with the
> macro location code, we could now tell that the constants INT_MAX and
> LONG_MAX come from different macro expansions in system headers, and
> avoid warning in this specific case, but that would be better done in
> a follow-up patch. Dodji, is that possible? how could it be done?
>
> Bootstrapped and regression tested.
>
> OK?
OK.
>
> 2012-05-05 Manuel López-Ibáñez
>
> PR c/43772
> c-family/
> * c-common.c (warn_logical_operator): Do not warn if either side
> is already true or false.
> testsuite/
> * c-c++-common/pr43772.c: New.
Re: [RFH / Patch] C++/52282
The NOP_EXPR are changing the "visible" types without changing
the representation; sometimes it is about lvalueness being thrown
away (which I suspect is the case here). I've always grumbled about not
having a uniform way of saying "convert this expression to type T
to an expression of type U".
On Sat, May 5, 2012 at 5:38 AM, Paolo Carlini wrote:
> Hi,
>
> I'm analyzing this PR and the various testcases which come with it. It looks
> like we have indeed two separate issues: one, which looks simpler, with
> decltype, leading to ICEs; a more complex one with constexpr. The former is
> about ADDR_EXPR unhandled in the finish_decltype_type switch for this
> testcase:
>
> template
> struct C
> {
> static constexpr decltype(V) c() { return V; }
> };
>
> struct D
> {
> static constexpr int d() { return 10; }
> };
>
> static_assert((C::c())() == 10, "oops");
>
> Would it make sense to just handle ADDR_EXPR too like in the patchlet below?
> It seems we are in this case too in the same situation which led to handling
> INTEGER_CST and PTRMEM_CST, or we have an issue with const-ness?
>
> The problems with constexpr look more nasty, are all about testcases similar
> to the above, many variants of it, like:
>
> template
> struct B
> {
> typedef T type;
> static constexpr type b() { return V; }
> };
>
> struct D
> {
> static constexpr int d() { return 10; }
> };
>
> static_assert((B::b())() == 10, "oops");
>
> which is rejected like:
>
> 52282.C:32:1: error: non-constant condition for static assertion
> static_assert((B::b())() == 10, "oops"); // line 30
>
> 52282.C:32:38: error: expression ‘D::d’ does not designate a constexpr
> function
> static_assert((B::b())() == 10, "oops"); // line 30
>
> For this kind of testcase I see a lot of NOP_EXPRs around, which I don't
> really understand and *appear* to confuse quite a bit code we have in the
> cxx_eval_* functions. For example, for the above, one crops up at the
> beginning of cxx_eval_call_expression:
>
> if (TREE_CODE (fun) != FUNCTION_DECL)
> {
> /* Might be a constexpr function pointer. */
> fun = cxx_eval_constant_expression (old_call, fun, allow_non_constant,
> /*addr*/false, non_constant_p);
> if (TREE_CODE (fun) == ADDR_EXPR)
> fun = TREE_OPERAND (fun, 0);
> }
>
> as the fun returned by cxx_eval_constant_expression and the logic handling
> ADDR_EXPR doesn't seem to work as designed. If I force a STRIP_NOPS the
> testcase is "accepted". But really I see something brittle about all these
> NOP_EXPRs and I'm looking for comments / hints. I don't think we just want
> to add STRIP_NOPS in a ton of places, which, if I understand correctly,
> would probably also imply the need to unshare_expr, but I don't have
> brilliant ideas right now...
>
> Another kind of weird issue we are facing is for:
>
> template
> struct W_ { static constexpr T value = V; };
>
> constexpr struct C {
> constexpr int c1() const { return 10; }
> } c;
>
> static_assert((c.*W_::value)() == 10, "oops");
>
> which leads to:
>
> ice.cpp:72:1: error: non-constant condition for static assertion
> static_assert((c.*W_::value)() == 10, "oops");
>
> ice.cpp:72:56: error: expression ‘0u’ does not designate a constexpr
> function
> static_assert((c.*W_::value)() == 10, "oops");
>
> note the '0u'! (the error message comes from the beginning of
> cxx_eval_call_expression) I guess these issues should not require major
> surgeries if a testcase like the latter but using, instead of W_:
>
> template
> struct W { static constexpr T value() { return V; } };
>
> works fine. In this case too, if I follow the long chain of cxx_eval_* I see
> a lot of NOP_EXPRs, but I'm not sure it's all there is to the issue, maybe
> we are just doing something wrong with the pointers...
>
> Thanks for any comment!
> Paolo.
>
>
>
>
Re: [Patch, fortran] PR 49010/24518 MOD/MODULO fixes, take 2
Janne Blomqvist writes: > - When the arguments are constant, use mpfr_fmod instead of the naive mpfr 2.3.1 doesn't have mpfr_fmod. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Re: [Patch, fortran] PR 49010/24518 MOD/MODULO fixes, take 2
On Sat, May 5, 2012 at 2:31 PM, Andreas Schwab wrote: > Janne Blomqvist writes: > >> - When the arguments are constant, use mpfr_fmod instead of the naive > > mpfr 2.3.1 doesn't have mpfr_fmod. I know, but since GCC requires at least mpfr 2.4.2 we're ok. -- Janne Blomqvist
Re: [Patch, fortran] PR 49010/24518 MOD/MODULO fixes, take 2
Janne Blomqvist writes: > On Sat, May 5, 2012 at 2:31 PM, Andreas Schwab wrote: >> Janne Blomqvist writes: >> >>> - When the arguments are constant, use mpfr_fmod instead of the naive >> >> mpfr 2.3.1 doesn't have mpfr_fmod. > > I know, but since GCC requires at least mpfr 2.4.2 we're ok. No, it doesn't. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Re: [Patch, fortran] PR 49010/24518 MOD/MODULO fixes, take 2
Andreas Schwab wrote: Janne Blomqvist writes: On Sat, May 5, 2012 at 2:31 PM, Andreas Schwab wrote: mpfr 2.3.1 doesn't have mpfr_fmod. I know, but since GCC requires at least mpfr 2.4.2 we're ok. No, it doesn't. From 4.8's ./configure: # If we have GMP, check the MPFR version. ... #if MPFR_VERSION < MPFR_VERSION_NUM(2,3,1) choke me #endif On the other hand, http://gcc.gnu.org/install/prerequisites.html states: "MPFR Library version 2.4.2 (or later)" Tobias
Re: [RFC] PR 53063 encode group options in .opt files
On Sat, 5 May 2012, Manuel L?pez-Ib??ez wrote: > Comments? My knowledge of awk is basically zero, so suggestions on > how to improve the code are very welcome. > > Should I cleanup the patch and submit it with a Changelog? Yes please. Some observations: * finish_options_generated should take an opts_set pointer and use that, not initializers of -1, to determine whether particular options were previously set. Thus you should be able to remove the Init(-1) for the options you move to the new scheme (obviously you need to make sure nothing else is checking for the -1 value or setting the relevant fields without also updating the _set information; I did a spot check for warn_unused_function and that looks OK, for example, as nothing sets it except the -Wunused-function option via the Var flag, and the code you are moving to the new system). * Rather than having the linear search in find_flags_by_name, build up a mapping from option names to numbers in opt-read.awk. (All awk arrays are actually associative arrays, you just need to put opt_numbers[$1] = n_opts at an appropriate point when each option is read in.) -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
Thanks for the hints. This is what I am currently bootstrapping+regtesting. It builds and works on a few manual tests. OK if it passes? 2012-05-05 Manuel López-Ibáñez PR c/53063 gcc/ * doc/options.texi (EnabledBy): Document. * opts.c (finish_options): Call finish_options_generated instead of handling some options explicitly. * optc-gen.awk: Handle EnabledBy. * opth-gen.awk: Declare finish_options_generated. * common.opt (Wuninitialized): Use EnabledBy. Delete Init. (Wunused-but-set-variable): Likewise. (Wunused-function): Likewise. (Wunused-label): Likewise. (Wunused-value): Likewise. (Wunused-variable): Likewise. * opt-read.awk: Create opt_numbers array. On 5 May 2012 16:22, Joseph S. Myers wrote: > On Sat, 5 May 2012, Manuel López-Ibáñez wrote: > >> Comments? My knowledge of awk is basically zero, so suggestions on >> how to improve the code are very welcome. >> >> Should I cleanup the patch and submit it with a Changelog? > > Yes please. Some observations: > > * finish_options_generated should take an opts_set pointer and use that, > not initializers of -1, to determine whether particular options were > previously set. Thus you should be able to remove the Init(-1) for the > options you move to the new scheme (obviously you need to make sure > nothing else is checking for the -1 value or setting the relevant fields > without also updating the _set information; I did a spot check for > warn_unused_function and that looks OK, for example, as nothing sets it > except the -Wunused-function option via the Var flag, and the code you are > moving to the new system). > > * Rather than having the linear search in find_flags_by_name, build up a > mapping from option names to numbers in opt-read.awk. (All awk arrays are > actually associative arrays, you just need to put opt_numbers[$1] = n_opts > at an appropriate point when each option is read in.) > > -- > Joseph S. Myers > jos...@codesourcery.com group-options.diff Description: Binary data
Ping: [Patch]
The following patch needs approval: http://gcc.gnu.org/ml/gcc-patches/2012-04/msg00961.html The target-specific part is already approved: http://gcc.gnu.org/ml/gcc-patches/2012-04/msg00976.html Thanks.
Re: Ping: [Patch]
On Sat, May 5, 2012 at 1:12 PM, Georg-Johann Lay wrote: > The following patch needs approval: > > http://gcc.gnu.org/ml/gcc-patches/2012-04/msg00961.html > > The target-specific part is already approved: > > http://gcc.gnu.org/ml/gcc-patches/2012-04/msg00976.html > > Thanks. The documentation part is OK as well. -- Gaby
Re: Ping: always supply a mode to plus_constant
On Thu, May 3, 2012 at 11:15 AM, Richard Sandiford wrote: > Ping for this patch to always supply a mode to plus_constant: > > http://gcc.gnu.org/ml/gcc-patches/2012-04/msg00892.html > > I've done a diff of the changes since the original test revision > (which was r186448 FWIW) and there's only been one plus_constant- > related change since then: Alan's rs6000 prologue/epilogue fixes. > I'd repeat the original testing before committing. > This caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53249 -- H.J.
Re: [PATCH] teach emit_store_flag to use clz/ctz
On Fri, 27 Apr 2012, Paolo Bonzini wrote: > > What about cost considerations? We only seem to have the general > > "branches are expensive" metric - but ctz/clz may be prohibitely expensive > > themselves, no? > > Yeah, that's a general problem with this kind of tricks. In general > however clz/ctz is getting less and less expensive, so I don't think > it is worrisome (at least at the beginning of stage 1). We can add > rtx_costs checks later. > > Among architectures I know, only i386 has an expensive bsf/bsr but > it also has sete/setne which GCC will use instead of this trick. > > Looking at rtx_costs, nothing seems to mark clz/ctz as prohibitively > expensive (Xtensa does, but only in the case when the optab handler > will not exist). I realize though that this is not a particularly > good statistic, since the compiler would not generate them out of > its hat until now. For the record: MIPS processors that implement CLZ/CLO (for some reason CTZ/CTO haven't been added to the architecture, but these operations can be cheaply transformed into CLZ/CLO) generally have a dedicated unit that causes no pipeline stall for these instructions even in the simplest pipeline designs like the M4K -- IOW they are issued at the usual one instruction per pipeline clock rate. Of course all MIPS processors have SLT too, so perhaps they won't benefit from your change either. Maciej
