This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFA: enable LRA for rs6000


On 13-04-11 5:04 PM, Vladimir Makarov wrote:
On 04/11/2013 02:32 PM, David Edelsohn wrote:
On Thu, Apr 11, 2013 at 1:30 PM, Vladimir Makarov <vmakarov@redhat.com> wrote:
Here is a patch to enable LRA for rs6000. The patch includes code changes
in rs6000 machine-dependent parts and in LRA parts.

I've added a new switch -mlra for rs6000 to make debugging LRA for rs6000 easier but not documented it as it will be gone at the end of stage1 (may be with ability to use LRA for rs6000 if the results are unsatisfactory). I have only one question about its default value. Currently by default LRA is used but if you prefer opposite I can reverse it. Please just let me know
your opinion.

   The patch was successfully bootstrapped and tested on pppc64 and
x86/x86-64.

   Are rs6000 parts ok for trunk?
Vlad,

Thanks for your work on LRA and your work to convert the rs6000 port
to use the new feature.

My main question about the rs6000 parts of the patch is: what is
toc_ok_p suppose to mean?

-      if (TARGET_TOC)
+      toc_ok_p = (lra_in_progress && TARGET_CMODEL != CMODEL_SMALL
+                        && small_toc_ref (x, VOIDmode));
+      if (TARGET_TOC && ! toc_ok_p)
   return false;

This is a change to the meaning of the test with no explanation or
comment.  At least the name is confusing because it seems to be
selecting a subset of valid TOC addressing forms. Medium and large
model TOC references before reload?

Also suffix "_ok" and "_p" seem redundant.

Maybe your intent is a boolean for "large" TOC that could be named
large_toc_p or large_toc_ok?

This is a hard question for me. Thanks for pointing this out, David. The code is 9 months old. It was introduced with lo_sum support for ppc:

http://gcc.gnu.org/ml/gcc-patches/2012-07/msg00260.html

Without this change, there are >3000 failures on GCC testsuite. If ppc target code says that the address

(lo_sum:DI (reg:DI <some pseudo>)
                (plus:DI (unspec:DI [
                            (symbol_ref:DI ("*.LANCHOR0") [flags 0x182])
                            (reg:DI 2 2)
                        ] UNSPEC_TOCREL)
                    (const_int 10 [0xa])))

is not legitimate lo_sum address, LRA tries to reload an address generating insn

<a pseudo> = (plus:DI (unspec:DI [
                         (symbol_ref:DI ("*.LANCHOR0") [flags 0x182])
                        (reg:DI 2 2)
                       ] UNSPEC_TOCREL)
              (const_int 10 [0xa]))

and that fails

I believe it was a quick hack for old address decomposition code which processed wrongly some addresses. I guess I forgot about this hack. Looking at this I don't like it now.

Probably we still need a more informative address decomposition which was rewritten by Richard Sandiford (and out of my control now). I'll investigate this more but probably another solution will be ready next week in the best case.

After thorough investigation I found that this code is still ok. The explanations are in the comment. Here is the modified version of the code taking you proposals into account

@@ -5701,19 +5705,31 @@ legitimate_lo_sum_address_p (enum machin

   if (TARGET_ELF || TARGET_MACHO)
     {
+      bool large_toc_ok;
+
       if (DEFAULT_ABI != ABI_AIX && DEFAULT_ABI != ABI_DARWIN && flag_pic)
        return false;
-      if (TARGET_TOC)
+      /* LRA don't use LEGITIMIZE_RELOAD_ADDRESS as it usually calls
+        push_reload from reload pass code. LEGITIMIZE_RELOAD_ADDRESS
+        recognizes some LO_SUM addresses as valid although this
+        function says opposite.  In most cases, LRA through different
+        transformations can generate correct code for address reloads.
+        It can not manage only some LO_SUM cases.  So we need to add
+        code analogous to one in rs6000_legitimize_reload_address for
+        LOW_SUM here saying that some addresses are still valid. */
+      large_toc_ok = (lra_in_progress && TARGET_CMODEL != CMODEL_SMALL
+                     && small_toc_ref (x, VOIDmode));
+      if (TARGET_TOC && ! large_toc_ok)
        return false;
       if (GET_MODE_NUNITS (mode) != 1)
        return false;
-      if (GET_MODE_SIZE (mode) > UNITS_PER_WORD
+      if (! lra_in_progress && GET_MODE_SIZE (mode) > UNITS_PER_WORD
          && !(/* ??? Assume floating point reg based on mode?  */
               TARGET_HARD_FLOAT && TARGET_FPRS && TARGET_DOUBLE_FLOAT
               && (mode == DFmode || mode == DDmode)))
        return false;

-      return CONSTANT_P (x);
+      return CONSTANT_P (x) || large_toc_ok;
     }

   return false;


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]