Re: [PATCH 2/2] asm inline

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Tue, Dec 04, 2018 at 03:30:47PM +0000, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hmm, so we allow top-level "asm volatile" in C++ but not C?

Apparently.  Evert top-level asm is effectively volatile, so this doesn't
really matter; but should we try to resolve the difference?

> Probably worth having tests to show that we (intentionally) don't
> allow top-level "asm inline".

Okay, I'll do this separately.

> > --- a/gcc/doc/extend.texi
> > +++ b/gcc/doc/extend.texi
> > @@ -8382,6 +8382,10 @@ various @option{-std} options, use @code{__asm__} instead of
> >  @item volatile
> >  The optional @code{volatile} qualifier has no effect. 
> >  All basic @code{asm} blocks are implicitly volatile.
> > +
> > +@item inline
> > +If you use the @code{inline} qualifier, then for inlining purposes the size
> > +of the asm is taken as the smallest size possible (@pxref{Size of an asm}).
> >  @end table
> >  
> >  @subsubheading Parameters
> 
> You need to update the syntax above too, which currently reads:
> 
> @example
> asm @r{[} volatile @r{]} ( @var{AssemblerInstructions} )
> @end example

That was modified in patch 1?

@example
asm @var{asm-qualifiers} ( @var{AssemblerInstructions} )
@end example

> > +/* Return true ASM_STMT ASM_STMT is an asm statement marked inline.  */
> > +
> > +static inline bool
> > +gimple_asm_inline_p (const gasm *asm_stmt)
> > +{
> > +  return (asm_stmt->subcode & GF_ASM_INLINE) != 0;
> > +}
> 
> Return true if ASM_STMT is ...

Heh, I copied this from the "volatile" one above.  I'll fix that one, too.

> (Or "Return true if asm statement ASM_STMT is marked inline", since gasm
> forces it to be an asm statement.)

Yup, same deal.

> > diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> > index 5aa782b..7e9ed99 100644
> > --- a/gcc/tree-inline.c
> > +++ b/gcc/tree-inline.c
> > @@ -4109,6 +4109,9 @@ estimate_num_insns (gimple *stmt, eni_weights *weights)
> >  	   with very long asm statements.  */
> >  	if (count > 1000)
> >  	  count = 1000;
> > +	/* If this asm is asm inline, count anything as minimum size.  */
> > +	if (gimple_asm_inline_p (as_a <gasm *> (stmt)))
> > +	  count = !!count;
> 
> FWIW, since Marc found it confusing too... I think MIN (count, 1) would
> show the intent more clearerly.  But that's just personal preference,
> no need to change.

MIN is a nice way to say it, thanks!  Better than "!!" or "!= 0" here.

Thanks for the review, I'm committing it with the changes above (later
today, after a final bootstrap),


Segher