| Message ID | or35x2b00d.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | ipa-sra is mostly disabled by -mlongcall | expand |
Hi, On Thu, Mar 11 2021, Alexandre Oliva wrote: > Several ipa-sra tests fail because -mlongcall on powerpc is a > TYPE_ATTRIBUTE, and those disable ipa-sra. > > This local workaround disables -mlongcall for the failing tests on > powerpc*-*-vxworks*, so they get a chance to run even in kernel mode. > > (AdaCore has -mlongcall enabled for powerpc*-wrs-vxworks* kernel-mode > targets.) > > This was regstrapped on x86_64-linux-gnu and ppc64-linux-gnu, and tested > with a cross to a ppc64-vxworks7r2 with -mlongcall enabled by default. > > I was leaning towards maintaing this change internal, but I thought it > wouldn't hurt to ask whether it's ok to install. > > I wonder whether it wouldn't be more desirable to have an alternate > implementation of -mlongcall on ppc that didn't conflict with IPA-SRA, > and IIRC with some scenarios involving C++ templates. when it comes to attributes, often IPA-SRA is over-eager to switch itself off, even though it could just carry on, which might also be this case. Unfortunately, I noticed that this "to be removed later" check is still there only at the end of November and did not manage to remove it before stage 4. I'll fix it in spring. Thanks for taking care of the testcases, I will use them for testing. Martin > > E.g., on ARM, -mlong-calls doesn't add an attribute to every function, > it just changes a default, that attributes short_call and long_call may > then override for specific functions. Would a similar implementation be > acceptable for ppc? > > > for gcc/testsuite/ChangeLog > > * gcc.dg/ipa/ipa-sra-12.c: Add -mno-longcall on ppc*-vxworks*. > * gcc.dg/ipa/ipa-sra-13.c: Likewise. > * gcc.dg/ipa/ipa-sra-15.c: Likewise. > * gcc.dg/ipa/ipa-sra-16.c: Likewise. > * gcc.dg/ipa/ipa-sra-17.c: Likewise. > * gcc.dg/ipa/ipa-sra-18.c: Likewise.
On Thu, Mar 11, 2021 at 9:19 AM Alexandre Oliva <oliva@adacore.com> wrote: > > > Several ipa-sra tests fail because -mlongcall on powerpc is a > TYPE_ATTRIBUTE, and those disable ipa-sra. > > This local workaround disables -mlongcall for the failing tests on > powerpc*-*-vxworks*, so they get a chance to run even in kernel mode. > > (AdaCore has -mlongcall enabled for powerpc*-wrs-vxworks* kernel-mode > targets.) > > This was regstrapped on x86_64-linux-gnu and ppc64-linux-gnu, and tested > with a cross to a ppc64-vxworks7r2 with -mlongcall enabled by default. > > I was leaning towards maintaing this change internal, but I thought it > wouldn't hurt to ask whether it's ok to install. > > I wonder whether it wouldn't be more desirable to have an alternate > implementation of -mlongcall on ppc that didn't conflict with IPA-SRA, > and IIRC with some scenarios involving C++ templates. > > E.g., on ARM, -mlong-calls doesn't add an attribute to every function, > it just changes a default, that attributes short_call and long_call may > then override for specific functions. Would a similar implementation be > acceptable for ppc? I wonder what the effect of this switch is - it's documented as affecting the call site (different call insn sequence by default), so the best representation would be an attribute on actual call stmts so inlining a non -mlongcall fn into a -mlongcall fn will properly inherit the setting of calls in the inlined function? So in C++ I should be able to write [[gnu::arm::longcall]] foo (); to get a single call forced as long-call (whatever this is used for in practice). I agree that a type attribute (it's a type attribute likely to also affect indirect calls) is a bit awkward but it does look better than just a global flag. Note that the long-standing issue in IPA is that IPA would need to know the semantics of attributes since it might need to modify them. One idea (partly implemented already I think) is to have a whitelist, aka, "this attribute doesn't need to be adjusted when parameters are split, removed or added or when the function is split or merged" - where for target specific attributes this would need a new target hook. Adjusting the testsuite just hides this missed optimization issue, so at least a bugreport with xfail keyword refering to the altered tests should be opened. Richard. > > > for gcc/testsuite/ChangeLog > > * gcc.dg/ipa/ipa-sra-12.c: Add -mno-longcall on ppc*-vxworks*. > * gcc.dg/ipa/ipa-sra-13.c: Likewise. > * gcc.dg/ipa/ipa-sra-15.c: Likewise. > * gcc.dg/ipa/ipa-sra-16.c: Likewise. > * gcc.dg/ipa/ipa-sra-17.c: Likewise. > * gcc.dg/ipa/ipa-sra-18.c: Likewise. > --- > gcc/testsuite/gcc.dg/ipa/ipa-sra-12.c | 1 + > gcc/testsuite/gcc.dg/ipa/ipa-sra-13.c | 1 + > gcc/testsuite/gcc.dg/ipa/ipa-sra-15.c | 1 + > gcc/testsuite/gcc.dg/ipa/ipa-sra-16.c | 1 + > gcc/testsuite/gcc.dg/ipa/ipa-sra-17.c | 1 + > gcc/testsuite/gcc.dg/ipa/ipa-sra-18.c | 1 + > 6 files changed, 6 insertions(+) > > diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-12.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-12.c > index 0cc76bde319c7..0d74c8f52eee2 100644 > --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-12.c > +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-12.c > @@ -1,5 +1,6 @@ > /* { dg-do run } */ > /* { dg-options "-O2 -fipa-sra -fdump-ipa-sra" } */ > +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ > > /* Check of a simple and transitive structure split. */ > > diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-13.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-13.c > index e8751dad67fcc..66083f040bad9 100644 > --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-13.c > +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-13.c > @@ -1,5 +1,6 @@ > /* { dg-do run } */ > /* { dg-options "-O2 -fipa-sra -fdump-ipa-sra" } */ > +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ > > /* Check of a by-reference structure split. */ > > diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-15.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-15.c > index aa13a94c7c064..98b7ae5d4f11b 100644 > --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-15.c > +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-15.c > @@ -1,5 +1,6 @@ > /* { dg-do run } */ > /* { dg-options "-O2 -fipa-sra -fdump-ipa-sra" } */ > +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ > > /* Check of a recursive by-reference structure split. The recursive functions > have to be pure right from the start, otherwise the current AA would detect > diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-16.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-16.c > index 2bffe297c7456..25f65fe1fc05a 100644 > --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-16.c > +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-16.c > @@ -1,5 +1,6 @@ > /* { dg-do compile } */ > /* { dg-options "-O2 -fipa-sra -fdump-ipa-sra -fdump-tree-optimized" } */ > +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ > > /* Testing removal of unused parameters in recursive calls. */ > > diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-17.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-17.c > index 9cb6367b37478..eaec0fcf90a91 100644 > --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-17.c > +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-17.c > @@ -1,5 +1,6 @@ > /* { dg-do compile } */ > /* { dg-options "-O2 -fdump-ipa-sra -fdump-tree-optimized" } */ > +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ > > #define DOIT > #define DONT > diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-18.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-18.c > index 3217b612231a2..1435ab228e3ae 100644 > --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-18.c > +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-18.c > @@ -1,5 +1,6 @@ > /* { dg-do compile } */ > /* { dg-options "-O2 -fdump-ipa-sra" } */ > +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ > > struct S > { > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > Vim, Vi, Voltei pro Emacs -- GNUlius Caesar
On Mar 11, 2021, Richard Biener <richard.guenther@gmail.com> wrote: > I wonder what the effect of this switch is - it's documented as > affecting the call site (different call insn sequence by default), so > the best representation would be an attribute on actual call stmts I don't think that would fulfill its purpose. My understanding is that the longcall attribute is intended to issue call sequences that assume the target of the call will be out of range for available PC-relative addressing modes in call instructions. It is also often the case that the callee address ends up in a contiguous address, rather than as multiple fragments of insns, which simplifies relocation processing. One use I'm familiar with is when different modules are known to be going to be linked separately, and dynamically loaded at disparate memory locations, so that PC-relative branches across modules won't do, and PLTs are not available. When building one module, you declare as requiring long call sequences any functions expected to be defined in other modules. Or you just use long calls throughout, taking a slight performance penalty out of intra-module call sequences, that could have used PC-relative calls. > I agree that a type attribute (it's a type attribute likely to also > affect indirect calls) is a bit awkward Indirect calls are pretty much long calls by definition, but I suppose the goal of retaining the long-call requests has to do with the possibility of sometimes resolving an indirect call back to a direct call. If it wasn't associated with the type, the information would be lost at the point of the function decay to pointer, but I gather if we succeed in resolving a pointer back to a unique declaration, we would be able to take a longcall annotation from that declaration. I'm probably missing something. > but it does look better than just a global flag. That possibility is not lost in the ARM implementation. What the ARM command-line flag does is change the default call sequence for that translation unit. It can still be overridden either way. What the PPC command-line flag does is to add the attribute to every single function decl's type. The representations are isomorphic, equivalent in terms of representation power, but the PPC attributes-all-over choice is wasteful memory-wise, and it sacrifices optimizations that throw their hands up when they find function type attributes. OTOH, it brings some simplification to the function type system. ARM's representation has 4 states (two attributes that can each be present or absent): one nonsensical (both short_ and long_call present), and one (both absent) mapping to either of the two others, depending on the default. On PPC, there are only two states: longcall present or absent, and AFAIK it offers no possibility of overriding a longcall default on a per-function basis, only globally for a translation unit, which sort of renders all the attributes it adds redundant.
On Thu, Mar 11, 2021 at 11:29:57AM -0300, Alexandre Oliva wrote: > On Mar 11, 2021, Richard Biener <richard.guenther@gmail.com> wrote: > > I wonder what the effect of this switch is - it's documented as > > affecting the call site (different call insn sequence by default), so > > the best representation would be an attribute on actual call stmts > > I don't think that would fulfill its purpose. That is how it already works though. The option sets a flag that gives the default for a function attribute. The function attribute is used at the call site: === void f(void); void g(void) __attribute__((longcall)); void h(void) { f(); g(); } static void (*i)(void) = f; static void (*j)(void) __attribute__((longcall)) = f; void k(void) { i(); j(); } === Note that in "k" the call via "i" is done as a direct call, and that via "j" uses a bctr. > My understanding is that the longcall attribute is intended to issue > call sequences that assume the target of the call will be out of range > for available PC-relative addressing modes in call instructions. Yes. > One use I'm familiar with is when different modules are known to be > going to be linked separately, and dynamically loaded at disparate > memory locations, so that PC-relative branches across modules won't do, > and PLTs are not available. And you need to call via function pointers *anyway*, so you do not need this attribute at all? It could matter if you do cross-module IPA optimisations, but you don't (do you? That will cause no end of fresh new problems!) > Indirect calls are pretty much long calls by definition, It's the other way around. Longcalls are implemented as indirect calls. > but I suppose > the goal of retaining the long-call requests has to do with the > possibility of sometimes resolving an indirect call back to a direct > call. You will save yourself a lot of problems by simply making that impossible. Makes such function pointers explicit in your code. > If it wasn't associated with the type, It isn't. It is a function attribute. > > but it does look better than just a global flag. The flag is just for setting a default, which often is handy (esp. for code that is too humonguous to compile without it: it is a simple flag to add, and your program was probably slow to begin with, being huge and all). Segher
On Thu, Mar 11, 2021 at 04:52:18AM -0300, Alexandre Oliva wrote: > Several ipa-sra tests fail because -mlongcall on powerpc is a > TYPE_ATTRIBUTE, and those disable ipa-sra. > > This local workaround disables -mlongcall for the failing tests on > powerpc*-*-vxworks*, so they get a chance to run even in kernel mode. > > (AdaCore has -mlongcall enabled for powerpc*-wrs-vxworks* kernel-mode > targets.) > > This was regstrapped on x86_64-linux-gnu and ppc64-linux-gnu, and tested > with a cross to a ppc64-vxworks7r2 with -mlongcall enabled by default. > > I was leaning towards maintaing this change internal, but I thought it > wouldn't hurt to ask whether it's ok to install. > > I wonder whether it wouldn't be more desirable to have an alternate > implementation of -mlongcall on ppc that didn't conflict with IPA-SRA, > and IIRC with some scenarios involving C++ templates. It has been this way for decades (well, since 2002, not *quite* multiple decades yet). It also is almost never necessary to use: not many programs have more than 32MB in a single module (and you do not need this for cross-module calls). > E.g., on ARM, -mlong-calls doesn't add an attribute to every function, > it just changes a default, that attributes short_call and long_call may > then override for specific functions. Would a similar implementation be > acceptable for ppc? That is how it works already! See the first entry on https://gcc.gnu.org/onlinedocs/gcc/PowerPC-Function-Attributes.html Given Martin's and Richard's replies, I think we should not take this patch. Sorry. Segher
On Thu, Mar 11, 2021 at 8:51 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Mar 11, 2021 at 04:52:18AM -0300, Alexandre Oliva wrote: > > Several ipa-sra tests fail because -mlongcall on powerpc is a > > TYPE_ATTRIBUTE, and those disable ipa-sra. > > > > This local workaround disables -mlongcall for the failing tests on > > powerpc*-*-vxworks*, so they get a chance to run even in kernel mode. > > > > (AdaCore has -mlongcall enabled for powerpc*-wrs-vxworks* kernel-mode > > targets.) > > > > This was regstrapped on x86_64-linux-gnu and ppc64-linux-gnu, and tested > > with a cross to a ppc64-vxworks7r2 with -mlongcall enabled by default. > > > > I was leaning towards maintaing this change internal, but I thought it > > wouldn't hurt to ask whether it's ok to install. > > > > I wonder whether it wouldn't be more desirable to have an alternate > > implementation of -mlongcall on ppc that didn't conflict with IPA-SRA, > > and IIRC with some scenarios involving C++ templates. > > It has been this way for decades (well, since 2002, not *quite* multiple > decades yet). It also is almost never necessary to use: not many > programs have more than 32MB in a single module (and you do not need > this for cross-module calls). > > > E.g., on ARM, -mlong-calls doesn't add an attribute to every function, > > it just changes a default, that attributes short_call and long_call may > > then override for specific functions. Would a similar implementation be > > acceptable for ppc? > > That is how it works already! See the first entry on > https://gcc.gnu.org/onlinedocs/gcc/PowerPC-Function-Attributes.html > > Given Martin's and Richard's replies, I think we should not take this > patch. Sorry. You could dg-skip-if -mlongcall if the option is explicit or XFAIL on some new target selector checking for its effect when it is enabled by default somehow. Of course given what Segher says I wonder why it's the default on vxworks... Richard. > > Segher
diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-12.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-12.c index 0cc76bde319c7..0d74c8f52eee2 100644 --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-12.c +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-12.c @@ -1,5 +1,6 @@ /* { dg-do run } */ /* { dg-options "-O2 -fipa-sra -fdump-ipa-sra" } */ +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ /* Check of a simple and transitive structure split. */ diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-13.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-13.c index e8751dad67fcc..66083f040bad9 100644 --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-13.c +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-13.c @@ -1,5 +1,6 @@ /* { dg-do run } */ /* { dg-options "-O2 -fipa-sra -fdump-ipa-sra" } */ +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ /* Check of a by-reference structure split. */ diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-15.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-15.c index aa13a94c7c064..98b7ae5d4f11b 100644 --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-15.c +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-15.c @@ -1,5 +1,6 @@ /* { dg-do run } */ /* { dg-options "-O2 -fipa-sra -fdump-ipa-sra" } */ +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ /* Check of a recursive by-reference structure split. The recursive functions have to be pure right from the start, otherwise the current AA would detect diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-16.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-16.c index 2bffe297c7456..25f65fe1fc05a 100644 --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-16.c +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-16.c @@ -1,5 +1,6 @@ /* { dg-do compile } */ /* { dg-options "-O2 -fipa-sra -fdump-ipa-sra -fdump-tree-optimized" } */ +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ /* Testing removal of unused parameters in recursive calls. */ diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-17.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-17.c index 9cb6367b37478..eaec0fcf90a91 100644 --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-17.c +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-17.c @@ -1,5 +1,6 @@ /* { dg-do compile } */ /* { dg-options "-O2 -fdump-ipa-sra -fdump-tree-optimized" } */ +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ #define DOIT #define DONT diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-18.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-18.c index 3217b612231a2..1435ab228e3ae 100644 --- a/gcc/testsuite/gcc.dg/ipa/ipa-sra-18.c +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-18.c @@ -1,5 +1,6 @@ /* { dg-do compile } */ /* { dg-options "-O2 -fdump-ipa-sra" } */ +/* { dg-additional-options "-mno-longcall" { target powerpc*-*-vxworks* } } */ struct S {