This is the mail archive of the
mailing list for the GCC project.
Re: [Patch, Fortran] PR fortran/35681: First part, fix ELEMENTAL dependency handling for MVBITS
- From: Daniel Kraft <d at domob dot eu>
- To: Paul Thomas <paul dot richard dot thomas at gmail dot com>
- Cc: Fortran List <fortran at gcc dot gnu dot org>, gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Sat, 01 Nov 2008 14:35:29 +0100
- Subject: Re: [Patch, Fortran] PR fortran/35681: First part, fix ELEMENTAL dependency handling for MVBITS
- References: <490605A9.firstname.lastname@example.org> <490B20FD.email@example.com> <490C502D.firstname.lastname@example.org>
Paul Thomas wrote:
Regtests fine on FC9/x86_i64. This is OK for trunk.
Committed as rev 141516. Seems Janus did commit exactly the same time
:D Thanks for the review Paul!
Have you had any thoughts on parentheses expressions yet?
Only very little... I'll comment on this from my point of view soon in
a new post.
Thanks for the patch
I've updated the patch described below to trunk of now (including the
trivial conflicts merge with Mikael's recent check-in) and run a new
regtest, no regressions on GNU/Linux-x86-32.
Daniel Kraft wrote:
working on PR fortran/35681, I've got some rather big patch now
handling part of the problem. What it exactly does:
1) Some tab-indentation formatting fixes as I came along, sorry for
those. I hope it is ok so.
2) When resolving a MVBITS intrinsic call, the code->resolved_sym
gets a dummy formal argument list with the correct INTENTs specified;
this is needed later for gfc_conv_elemental_dependencies.
3) gfc_code got a new member "resolved_isym" that tracks calls to
intrinsic procedures, so we can later check if some call is to
intrinsic MVBITS. This got a little ugly and would be probably nicer
to union it (and possibly "resolved_sym", too) with actual, but that
would probably introduce a lot of changes to existing code pieces.
4) gfc_trans_allocate_array_storage (or what it is called) got a new
argument `initial' that allows to initialize the created storage from
some other array (this is done using a combination of internal_pack
and memcpy if it was already packed, I hope I got this all right).
This is used for gfc_trans_create_temp_array to allow initializing
the new temporary. Here is (probably) most of the "critical" changes.
5) For calls to intrinsic MVBITS, I enabled dependency checking using
gfc_conv_elemental_dependencies and made this routine aware of
INTENT(INOUT) arguments that use the new initialization feature to
copy over the initial content of the mirrored array to the created
6) I could not find a test to verify this (not even one that uses
gfc_conv_elemental_dependencies) in a quick trial, but I believe the
handling of the temporary there was wrong, in that it was free'd (if
allocated on the heap) *before* it was used with internal_unpack,
because gfc_trans_create_temp_array added the temporary clean-up code
to se->post and the unpack-call was added to se->post later. In my
opinion, this is some rather general problem with how post-commands
are usually added to other post blocks; shouldn't they be added to
the top usually rather than to the bottom, to get some sort of
"nested" scope with inner most pairs of pre/post? Well, for now I
changed this behaviour inside gfc_conv_elemental_dependencies, which
corrected problems I got with MVBITS tests.
This enabled the (valid) tests in the PR to run, but only with
modifying them slightly by removing the parentheses around the first
argument (so it is not an expression; that will be part 2 of this
fix). As I understand it, this is valid in case of MVBITS but not
for any other ELEMENTAL subroutine, right? This is why I added the
check for whether some call is to MVBITS. I guess the rationale why
the compiler is not required to create temporaries for all such
ELEMENTAL calls (and they are invalid instead) is performance?
gfortran could handle those calls well in addition to only MVBITS
calls simply if I take this conditional check out, but then we might
generate temporaries for cases where the user knows no one is needed
and the code is valid but the compiler can't figure it out.
I hope I got this one at least somewhat clear... What do you think
about it? Currently regression-testing on GNU/Linux-x86-32, but I
don't expect any (a very similar patch worked fine before).
To go: Hea-Kni-Mon-Pri-Ran-Tou