Re: [PATCH 2/3] Enabled LRA for ia64.

["René Rebe" <rene@exactcode.de>]

[-- Attachment #1: Type: text/plain, Size: 7203 bytes --]


On Jun 12, 2024, at 15:00, Richard Biener <rguenther@suse.de> wrote:
> 
> On Wed, 12 Jun 2024, René Rebe wrote:
> 
>> Hi,
>> 
>>> On Jun 12, 2024, at 13:01, Richard Biener <rguenther@suse.de> wrote:
>>> 
>>> On Wed, 12 Jun 2024, Rene Rebe wrote:
>>>> 
>>>> gcc/
>>>>       * config/ia64/ia64.cc: Enable LRA for ia64.
>>>>       * config/ia64/ia64.md: Likewise.
>>>>       * config/ia64/predicates.md: Likewise.
>>> 
>>> That looks simple enough.  I cannot find any copyright assignment on
>>> file with the FSF so you probably want to contribute to GCC under
>>> the DCO (see https://gcc.gnu.org/dco.html), in that case please post
>>> patches with Signed-off-by: tags.
>> 
>> If it helps for the future, I can apply for copyright assignment, too.
> 
> It's not a requirement - you as contributor get the choice under
> which legal framework you contribute to GCC, for the DCO there's
> the formal requirement of Signed-off-by: tags.
> 
>>> For this patch please state how you tested it, I assume you
>>> bootstrapped GCC natively on ia64-linux and ran the testsuite.
>>> I can find two gcc-testresult postings, one appearantly with LRA
>>> and one without?  Both from May:
>>> 
>>> https://sourceware.org/pipermail/gcc-testresults/2024-May/816422.html
>>> https://sourceware.org/pipermail/gcc-testresults/2024-May/816346.html
>> 
>> Yes, that are the two I quoted in the patch cover letter.
>> 
>> 	https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654321.html
>> 
>>> somehow for example libstdc++ summaries were not merged, it might
>>> be you do not have recent python installed on the system?  Or you
>>> didn't use contrib/test_summary to create those mails.  It would be
>>> nice to see the difference between LRA and not LRA in the testresults,
>>> can you quote that?
>> 
>> We usually cross-compile gcc, but also ran natively for the testsuite.
>> Given the tests run quite long natively on the hardware we currently
>> have, I summed the results them up in the cover letter. I would assume
>> that shoudl be enough to include with a note the resulting kernel and
>> user-space world was booted and worked without issues?
> 
> I specifically wondered if bootstrap with LRA enabled succeeds.
> That needs either native or emulated hardware.  I think we consider
> ia64-linux a host platform and not only a cross compiler target.

With “also ran” I meant to say we did both, our T2 framework usually
boot-straps everything cross compiled, but also supports native in-system
compilation. Frank also manually natively bootstrapped gcc and ran the
testsuite natively on ia64.

>> If so, I’ll just resend with the additional information added.
> 
> For the LRA enablement patch the requirement is that patches should
> state how they were tested - usually you'll see sth like
> 
> Boostrapped and tested on x86_64-unknown-linux-gnu.
> 
> In your case it was
> 
> Cross-built from x86_64-linux(?) to ia64-linux, natively tested

OK - I include the details in v2.

> not sure how you exactly did this though?  I've never tried
> testing of a canadian-cross tree - did you copy the whole build
> tree over from the x86 to the ia64 machine?

IIRC the testsuite did not just work copying the canadian-cross.
I did run the testsuite from the cross-compiled gcc using a ssh
based dejagnu board config, but Frank also did fully bootstrap and
ran the testsuite natively.

Thanks,
	René

> Thanks,
> Richard.
> 
>> Thank you so much,
>> 	René
>> 
>>> Thanks,
>>> Richard.
>>> 
>>>> ---
>>>> gcc/config/ia64/ia64.cc       | 7 ++-----
>>>> gcc/config/ia64/ia64.md       | 4 ++--
>>>> gcc/config/ia64/predicates.md | 2 +-
>>>> 3 files changed, 5 insertions(+), 8 deletions(-)
>>>> 
>>>> diff --git a/gcc/config/ia64/ia64.cc b/gcc/config/ia64/ia64.cc
>>>> index ac3d56073ac..d189bfb2cb4 100644
>>>> --- a/gcc/config/ia64/ia64.cc
>>>> +++ b/gcc/config/ia64/ia64.cc
>>>> @@ -618,9 +618,6 @@ static const scoped_attribute_specs *const ia64_attribute_table[] =
>>>> #undef TARGET_LEGITIMATE_ADDRESS_P
>>>> #define TARGET_LEGITIMATE_ADDRESS_P ia64_legitimate_address_p
>>>> 
>>>> -#undef TARGET_LRA_P
>>>> -#define TARGET_LRA_P hook_bool_void_false
>>>> -
>>>> #undef TARGET_CANNOT_FORCE_CONST_MEM
>>>> #define TARGET_CANNOT_FORCE_CONST_MEM ia64_cannot_force_const_mem
>>>> 
>>>> @@ -1329,7 +1326,7 @@ ia64_expand_move (rtx op0, rtx op1)
>>>> {
>>>>  machine_mode mode = GET_MODE (op0);
>>>> 
>>>> -  if (!reload_in_progress && !reload_completed && !ia64_move_ok (op0, op1))
>>>> +  if (!lra_in_progress && !reload_completed && !ia64_move_ok (op0, op1))
>>>>    op1 = force_reg (mode, op1);
>>>> 
>>>>  if ((mode == Pmode || mode == ptr_mode) && symbolic_operand (op1, VOIDmode))
>>>> @@ -1776,7 +1773,7 @@ ia64_expand_movxf_movrf (machine_mode mode, rtx operands[])
>>>> }
>>>>    }
>>>> 
>>>> -  if (!reload_in_progress && !reload_completed)
>>>> +  if (!lra_in_progress && !reload_completed)
>>>>    {
>>>>      operands[1] = spill_xfmode_rfmode_operand (operands[1], 0, mode);
>>>> 
>>>> diff --git a/gcc/config/ia64/ia64.md b/gcc/config/ia64/ia64.md
>>>> index 698e302081e..d485acc0ea8 100644
>>>> --- a/gcc/config/ia64/ia64.md
>>>> +++ b/gcc/config/ia64/ia64.md
>>>> @@ -2318,7 +2318,7 @@
>>>> (match_operand:DI 3 "register_operand" "f"))
>>>> (match_operand:DI 4 "nonmemory_operand" "rI")))
>>>>   (clobber (match_scratch:DI 5 "=f"))]
>>>> -  "reload_in_progress"
>>>> +  "lra_in_progress"
>>>>  "#"
>>>>  [(set_attr "itanium_class" "unknown")])
>>>> 
>>>> @@ -3407,7 +3407,7 @@
>>>>  (match_operand:DI 2 "shladd_operand" "n"))
>>>> (match_operand:DI 3 "nonmemory_operand" "r"))
>>>> (match_operand:DI 4 "nonmemory_operand" "rI")))]
>>>> -  "reload_in_progress"
>>>> +  "lra_in_progress"
>>>>  "* gcc_unreachable ();"
>>>>  "reload_completed"
>>>>  [(set (match_dup 0) (plus:DI (mult:DI (match_dup 1) (match_dup 2))
>>>> diff --git a/gcc/config/ia64/predicates.md b/gcc/config/ia64/predicates.md
>>>> index 01a4effd339..85f5380e734 100644
>>>> --- a/gcc/config/ia64/predicates.md
>>>> +++ b/gcc/config/ia64/predicates.md
>>>> @@ -347,7 +347,7 @@
>>>>  allows reload the opportunity to avoid spilling addresses to
>>>>  the stack, and instead simply substitute in the value from a
>>>>  REG_EQUIV.  We'll split this up again when splitting the insn.  */
>>>> - if (reload_in_progress || reload_completed)
>>>> + if (lra_in_progress || reload_completed)
>>>> return true;
>>>> 
>>>> /* Some symbol types we allow to use with any offset.  */
>>>> 
>>> 
>>> -- 
>>> Richard Biener <rguenther@suse.de>
>>> SUSE Software Solutions Germany GmbH,
>>> Frankenstrasse 146, 90461 Nuernberg, Germany;
>>> GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
>> 
>> 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH,
> Frankenstrasse 146, 90461 Nuernberg, Germany;
> GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)

-- 
ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
http://exactcode.com | http://exactscan.com | http://ocrkit.com