From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 127255 invoked by alias); 9 Dec 2016 14:30:16 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 127067 invoked by uid 89); 9 Dec 2016 14:30:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=U*dmalcolm, sk:dmalcol, dmalcolmredhatcom, dmalcolm@redhat.com X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 09 Dec 2016 14:30:00 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 23406707; Fri, 9 Dec 2016 06:29:58 -0800 (PST) Received: from localhost (e105548-lin.manchester.arm.com [10.45.32.67]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 9FB283F220; Fri, 9 Dec 2016 06:29:57 -0800 (PST) From: Richard Sandiford To: David Malcolm Mail-Followup-To: David Malcolm ,, richard.sandiford@arm.com Cc: Subject: Re: [7/67] Add scalar_float_mode References: <87h96dp8u6.fsf@e105548-lin.cambridge.arm.com> <87mvg5ntv3.fsf@e105548-lin.cambridge.arm.com> <1481293184.16607.25.camel@redhat.com> Date: Fri, 09 Dec 2016 14:30:00 -0000 In-Reply-To: <1481293184.16607.25.camel@redhat.com> (David Malcolm's message of "Fri, 9 Dec 2016 09:19:44 -0500") Message-ID: <877f79dvks.fsf@e105548-lin.cambridge.arm.com> User-Agent: Gnus/5.130012 (Ma Gnus v0.12) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2016-12/txt/msg00858.txt.bz2 David Malcolm writes: > On Fri, 2016-12-09 at 12:56 +0000, Richard Sandiford wrote: >> This patch adds a scalar_float_mode class, which wraps a mode enum >> that is known to satisfy SCALAR_FLOAT_MODE_P. Things like "SFmode" >> now give a scalar_float_mode object instead of a machine_mode. >> This in turn needs a change to the real.h format_helper, so that >> it can accept both machine_modes and scalar_float_modes. > > [...] > >> diff --git a/gcc/machmode.h b/gcc/machmode.h >> index 44a8ad4..996f991 100644 >> --- a/gcc/machmode.h >> +++ b/gcc/machmode.h >> @@ -29,6 +29,16 @@ extern const unsigned short >> mode_unit_precision[NUM_MACHINE_MODES]; >> extern const unsigned char mode_wider[NUM_MACHINE_MODES]; >> extern const unsigned char mode_2xwider[NUM_MACHINE_MODES]; >> >> +/* Allow direct conversion of enums to specific mode classes only >> + when USE_ENUM_MODES is defined. This is only intended for use >> + by gencondmd, so that it can tell more easily when .md conditions >> + are always false. */ >> +#ifdef USE_ENUM_MODES >> +#define PROTECT_ENUM_CONVERSION public >> +#else >> +#define PROTECT_ENUM_CONVERSION protected >> +#endif >> + >> /* Get the name of mode MODE as a string. */ >> >> extern const char * const mode_name[NUM_MACHINE_MODES]; >> @@ -237,6 +247,85 @@ opt_mode::exists (U *mode) const >> return false; >> } >> >> +/* Return true if mode M has type T. */ >> + >> +template >> +inline bool >> +is_a (machine_mode_enum m) >> +{ >> + return T::includes_p (m); >> +} >> + >> +/* Assert that mode M has type T, and return it in that form. */ >> + >> +template >> +inline T >> +as_a (machine_mode_enum m) >> +{ >> + gcc_checking_assert (T::includes_p (m)); >> + return T::from_int (m); >> +} >> + >> +/* Convert M to an opt_mode. */ >> + >> +template >> +inline opt_mode >> +dyn_cast (machine_mode_enum m) >> +{ >> + if (T::includes_p (m)) >> + return T::from_int (m); >> + return opt_mode (); >> +} >> + >> +/* Return true if mode M has type T, storing it as a T in *RESULT >> + if so. */ >> + >> +template >> +inline bool >> +is_a (machine_mode_enum m, U *result) >> +{ >> + if (T::includes_p (m)) >> + { >> + *result = T::from_int (m); >> + return true; >> + } >> + return false; >> +} >> + >> +/* Represents a machine mode that is known to be a >> SCALAR_FLOAT_MODE_P. */ >> +class scalar_float_mode > > Should this be a subclass of machine_mode? Or does this lead to name > clashes? (e.g. for "from_int") > > (Similar questions for the other more specialized wrapper classes; it > looks like you have a set of independent wrapper classes around the > machine_mode_enum, without using inheritance, with a trip through the > enum needed to convert between wrappers) Making it a subclass would be the natural thing to do if the class actually defined the mode (and if we for example used pointers to the definitions to represent the mode). With wrappers it's kind-of the "wrong way round" though: having scalar_int_mode a subclass of machine_mode would allow things like: void foo (machine_mode *x) { *x = SFmode; } void bar () { scalar_int_mode y; foo (&y); ... } and so isn't type-safe. >> +{ >> +public: >> + ALWAYS_INLINE scalar_float_mode () {} >> + ALWAYS_INLINE operator machine_mode_enum () const { return m_mode; >> } >> + >> + static bool includes_p (machine_mode_enum); >> + static scalar_float_mode from_int (int i); >> + >> +PROTECT_ENUM_CONVERSION: >> + ALWAYS_INLINE scalar_float_mode (machine_mode_enum m) : m_mode (m) >> {} > > Should this ctor have some kind of: > gcc_checking_assert (SCALAR_FLOAT_MODE_P (m)); > or somesuch? (or does that slow down the debug build too much?) The idea was to have the asserts in as_a <> calls instead, which is the point at which forcible conversion occurs. These routines are for cases where we've already done the check or know due to static type checking that a check isn't needed. >> + >> +protected: >> + machine_mode_enum m_mode; >> +}; >> + >> +/* Return true if M is a scalar_float_mode. */ >> + >> +inline bool >> +scalar_float_mode::includes_p (machine_mode_enum m) >> +{ >> + return SCALAR_FLOAT_MODE_P (m); >> +} >> + >> +/* Return M as a scalar_float_mode. This function should only be >> used by >> + utility functions; general code should use as_a instead. */ >> + >> +ALWAYS_INLINE scalar_float_mode >> +scalar_float_mode::from_int (int i) >> +{ >> + return machine_mode_enum (i); >> +} > > ...or put an assertion in here? (I *think* it's the only public > function that can (implicitly) call the ctor without checking the > invariant) Same idea here: this should only be called when no check is needed, and like you say shouldn't have an assert for speed reasons. The number of callers to from_int is small and so it's relatively easy to check that the calls are safe. > Hope this is constructive Yeah, definitely :-) Thanks, Richard