On Fri, 2017-03-10 at 09:24 +0000, Kyrill Tkachov wrote:
> On 10/03/17 06:24, Jakub Jelinek wrote:
> > On Thu, Mar 09, 2017 at 12:45:25PM -0500, David Malcolm wrote:
> > > gcc/ChangeLog:
> > > PR translation/79923
> > > * auto-profile.c (get_combined_location): Convert leading
> > > character of diagnostics to lower case and remove trailing
> > > period.
> > > (read_profile): Likewise for various diagnostics.
> > > * config/arm/arm-builtins.c (arm_expand_builtin): Remove
> > > trailing
> > > period from various diagnostics.
> > > * config/arm/arm.c (arm_option_override): Likewise.
> > > * config/msp430/msp430.c (msp430_expand_delay_cycles):
> > > Likewise.
> > > (msp430_expand_delay_cycles): Likewise.
> > Mostly ok, but for
> >
> > > --- a/gcc/config/arm/arm-builtins.c
> > > +++ b/gcc/config/arm/arm-builtins.c
> > > @@ -2990,60 +2990,60 @@ arm_expand_builtin (tree exp,
> > > && (imm < 0 || imm > 32))
> > > {
> > > if (fcode == ARM_BUILTIN_WRORHI)
> > > - error ("the range of count should be in 0 to 32.
> > > please check the intrinsic _mm_rori_pi16 in code.");
> > > + error ("the range of count should be in 0 to 32.
> > > please check the intrinsic _mm_rori_pi16 in code");
> > I wonder if this shouldn't use a semicolon space in the middle
> > instead of dot space space (many times in the same file).
>
> Is there a convention in GCC to use semicolons?
> I'm okay with changing it to a semicolon (it's slightly better IMO)
> as long as it's consistent
> with the style GCC uses.
>
> > Also, for the benefit of translators, this might be better done as
> > error ("the range of count should be in 0 to 32; please
> > check the intrinsic %s in code",
> > "_mm_rori_pi16");
> > so that there are fewer of these messages.
> > Adding some ARM folks on this.
>
> These iWMMXt builtins haven't been touched in ages and could do with
> some TLC in general.
> For example, I'm not a fan of having all these "please check the
> intrinsic ..." messages.
> If we've got a reference to the tree we're expanding, isn't there
> some kind of error function
> that will point to the location in the source that's causing the
> error? I'd rather use that than
> hardcoding the intrinsic names. This would also allow us to collapse
> all these
> if (fcode == <...>)
> error (...);
> else if (fcode == <...>)
> error (...);
> else if ...
>
> constructs.
>
> While we're at it:
>
> if (fcode == ARM_BUILTIN_WSRLHI)
> - error ("the count should be no less than 0. please
> check the intrinsic _mm_srli_pi16 in code.");
> + error ("the count should be no less than 0. please
> check the intrinsic _mm_srli_pi16 in code");
>
>
> Let's use "the count should be a non-negative integer" to be
> consistent with the error reporting
> for UInteger error messages.
>
> > Perhaps commit everything except arm-builtins.c separately and deal
> > with
> > this part with the ARM folks?
>
> I agree. David, if you want to clean up the error reporting in these
> intrinsics as a separate patch I'd be grateful.
> Otherwise, could you please open a bugzilla ticket so we can track
> this?
I started looking at this, but realized I don't have the arm ISA
expertise to touch this code (sorry), so I filed
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79995
instead.
