Segher Boessenkool <seg...@kernel.crashing.org> wrote:
> Hi!
>
> On Mon, Oct 14, 2019 at 07:18:11PM -0500, Peter Bergner wrote:
>> On 10/14/19 2:57 PM, Segher Boessenkool wrote:
>>> On Mon, Oct 14, 2019 at 06:35:06PM +0200, Richard Biener wrote:
>>>> The general case should be that if the caller ISA supports the callee one
>>>> then inlining is OK. If this is not wanted in some cases then there are
>>>> options like using a noinline attribute.
>>>
>>> I agree, and that is what we already do afaik.
>>
>> I agree on the making sure the caller's ISA supports the callee's ISA
>> before allowing inlining...and I think that's what our code it "trying"
>> to do. It just has some bugs.
>
> It says that it wants the callee ISA to be a subset of the caller ISA,
> and that is what it does. Which is exactly what we want. But as the
> PR points out it is *also* useful to not inline callees that explicitly
> disabled some ISA feature the caller has. Which we never promissed, but
> perhaps we should.
>
>> I don't think sprinkling noinline's around will work given LTO can
>> inline random functions from one object file into a function in another
>> object file and we have no idea what the options were used for both files.
>> I think that would just force us to end up putting nolines on all fuctions
>> that might be compiled with LTO.
>
> I think LTO should handle noinline attributes just fine, it would be a
> much bigger problem than this PR if it doesn't!
>
>
> Missing date+name+address here; and missing PR ref.
>
>> gcc/
>> * config/rs6000/rs6000.c (rs6000_can_inline_p): Handle explicit
>> options.
>
> "Prohibit inlining if the callee explicitly disables some isa_flags the
> caller has", or something like that? A few more words please.
>
>> --- gcc/testsuite/gcc.target/powerpc/pr70010.c (nonexistent)
>> +++ gcc/testsuite/gcc.target/powerpc/pr70010.c (working copy)
>> @@ -0,0 +1,21 @@
>> +/* { dg-do compile { target { powerpc*-*-* } } } */
>
> Just
> /* { dg-do compile } */
> please: everything in gcc.target checks for powerpc*-*-* always (and
> compile is the default, but it's nice documentation).
>
>> +/* { dg-skip-if "" { powerpc*-*-darwin* } } */
>
> You don't need this I think. If this test cannot work on Darwin for
> some reason, the Darwin maintainers will take care of it. Unless you
> *know* they want (or need) the skip here?
Right, it should not be needed because ..
>> +/* { dg-require-effective-target powerpc_vsx_ok } */
… this ^ should DTRT for Darwin too. If not, then that’s the place I want
fix it.
>> +/* { dg-options "-O2 -finline" } */
>
> -finline does nothing; it isn't even documented, only -fno-inline is.
>
>> +/* { dg-final { scan-assembler "bl vadd_no_vsx" } } */
>
> Please use
> /* { dg-final { scan-assembler "\mbl vadd_no_vsx\M" } } */
> so that it won't match "rldibl vadd_no_vsx", etc. Doesn't matter much
> for this testcase, but it's a good habit (and using it makes me not
> comment about it, also useful ;-) )
>
> Okay for trunk with those tweaks. Thanks!
>
>
> Segher
