Re: [Patch, Fortran] PR fortran/35681: First part, fix ELEMENTAL dependency handling for MVBITS

[Daniel Kraft <d@domob.eu>]

Paul Thomas wrote:
> Daniel,
> 
> 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.

Cheers,
Daniel

> Thanks for the patch
> 
> Paul
>> Hi,
>>
>> 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.
>>
>> Cheers,
>> Daniel
>>
>> 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 
>>> temporary.
>>>
>>> 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).
>>>
>>> Cheers,
>>> Daniel
>>>
>>
>>
> 
> 


-- 
Done:  Arc-Bar-Cav-Rog-Sam-Val-Wiz
To go: Hea-Kni-Mon-Pri-Ran-Tou