Re: [PATCH v2] fortran: add INLINE and ALWAYS_INLINE attributes

Am 02.06.26 um 10:07 PM schrieb Harald Anlauf:
> Hi Henri!
> 
> Am 02.06.26 um 1:15 PM schrieb Henri Menke:
>> Hi Harald,
>>
>> Thanks for the quick feedback.  Please find the patch for v2 attached.
>>
>> Let me briefly address the requested changes point-by-point.
>>
>> - Renamed FORCEINLINE to ALWAYS_INLINE and added a plain INLINE
>> attribute.  INLINE only sets DECL_DECLARED_INLINE_P, while
>> ALWAYS_INLINE additionally sets DECL_DISREGARD_INLINE_LIMITS.
>>
>> - The new EXT_ATTR_INLINE and EXT_ATTR_ALWAYS_INLINE values are now
>> appended at the end of the enum, before EXT_ATTR_LAST, so the existing
>> bit positions in the module-file bitmask are preserved.  I also added a
>> source comment before the enum noting that new attributes must be
>> appended for this reason.
>>
>> - The mutual-exclusion check now covers both INLINE and ALWAYS_INLINE
>> against NOINLINE.
>>
>> While implementing these changes, I have also noticed a difference from
>> the C always_inline attribute.  Because ALWAYS_INLINE sets
>> DECL_DISREGARD_INLINE_LIMITS directly rather than attaching an
>> always_inline attribute, it does not force inlining at -O0.  For now I
>> have left it at that, because attaching an attribute to the declaration
>> would make this patch a bit more complicated.  If you would prefer the
>> exact C semantics I am happy to attach the real attribute instead.
> 
> No, your current solution is really fine with me.
> 
>> One note on the testsuite.  The ALWAYS_INLINE test simply checks that
>> the procedure is inlined at -O.  The plain INLINE test needs more care,
>> because a small procedure is inlined at -O2 regardless of the
>> attribute.  That test therefore switches off automatic inlining with -
>> fno-inline-small-functions -fno-inline-functions -fno-inline-functions-
>> called-once and then verifies that the INLINE attribute alone re-
>> enables inlining for that one procedure.  I added a comment in the test
>> explaining this, but I worry about the robustness of this check,
>> especially on other platforms that I cannot test.
> 
> This is also fine.
> 
> There is only one thing I noted while playing with your new testcase
> inline_2.f90: depending on the ordering of the lines
> 
> !GCC$ ATTRIBUTES inline :: bar
> !GCC$ ATTRIBUTES noinline :: bar
> 
> one gets a warning that either looks correct or confusing.
> The way you've chosen it for the testcase I get:
> 
>     13 | !GCC$ ATTRIBUTES noinline :: bar  ! { dg-warning "INLINE. 
> at .1. is incompatible with .NOINLINE." }
>        |                                 1
> Warning: Attribute »INLINE« at (1) is incompatible with »NOINLINE« for 
> »bar« and will be ignored
> 
> This is because the order in which the attributes are processed
> is not taken into account by your new checks.
> 
> Anyway, I think this is rather a cosmetic issue and not a real problem.
> 
> Besides, there is a minor whitespace issue your can check yourself:
> 
> % ./contrib/check_GNU_style.sh 0001-fortran-add-INLINE-and- 
> ALWAYS_INLINE-attributes.patch
> 
> Blocks of 8 spaces should be replaced with tabs.
> 83:+  { "inline",       EXT_ATTR_INLINE,       NULL        },
> 84:+  { "always_inline",EXT_ATTR_ALWAYS_INLINE,NULL        },
> 
> It is hard to see without this hint, but after "NULL" there
> shall be a tab.  I can fix this myself.
> 
>> Bootstrapped and regtested on x86_64-pc-linux-gnu with --enable-
>> languages=c,c++,fortran, no regressions.
> 
> If nobody else speaks up, particularly w.r.t. the warnings mentioned
> above, I'll commit for you tomorrow.
> 
> Thanks for the patch!
> 
> Harald

No further comments, so pushed as r17-1274-ga3de5985947287 .

Thanks,
Harald