search

stfc / PSyclone

Domain-specific compiler and code transformation system for Finite Difference/Volume/Element Earth-system models in Fortran
BSD 3-Clause "New" or "Revised" License
105 stars 28 forks source link

ArrayRange2Trans should ignore character typed arrays (or PSyclone default script should) #2510

Closed LonelyCat124 closed 6 months ago

LonelyCat124 commented 7 months ago

There are some files in Socrates that have strings accessed using array range notation (which in a lot of cases just seems like bad code, but not important) that automatically running PSyclone on results in conversion to loops.

My feeling is that PSyclone (by default) shouldn't convert character arrays to loops when just running over files (as this is likely to be initialisation and not useful), and I'm not sure if this is something we might disable in the Transformation or in the example PSyclone scripts? One example in Socrates that it naively would do this on is:

      DO k=0, i_end-i_begin
        word(16-k:16-k)=string(i_end-k:i_end-k)
      ENDDO

The easy thing is to just skip over applying transformations to a lot of files, however I think in general converting string manipulation on ranges to loops might be strange.

@arporter @sergisiso what do you think?

Edit: This is probably more a examples script thing as opposed to a transformation thing?

arporter commented 7 months ago

I agree. The reason we convert array notation to loops is to allow us to apply other directives to the loop. I can't think of any case where that would make sense for a character string. My feeling is that we should extend the various transformations to check the datatype of either side of an assignment and if it is char, to reject it. We have the necessary functionality to do that now. ArrayRange2Loop is the obvious one but probably we also want to exclude character manipulation from within OpenACC/OpenMP regions.

In general, it's always best if we can get new functionality into PSyclone itself (where it is documented, tested etc.) rather than adding it to the examples scripts. I'll be the first to admit that we've got some way to go to reduce the size of those scripts :-)

LonelyCat124 commented 7 months ago

Ok, I can start on this for this transformation and make follow ups for others later.

My personal feeling is that we should allow this behaviour on force=True or allow_strings=True or similar as we might want the behaviour for converting fortran code into other languages or other backends.

arporter commented 7 months ago

Belatedly realised that this is a duplicate of #2317. Still, at least we fix two for the price of one :-)