(Closes #2499) scalarization transformation implementation

[LonelyCat124]

First implementation is here. I have unit tests for the private routines, I need to test it with code examples doing the full transformation properly but in theory its implemented here for now.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (6f618c9) to head (ed75417).
Report is 351 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2563   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         357      355    -2     
  Lines       48332    48402   +70     
=======================================
+ Hits        48266    48339   +73     
+ Misses         66       63    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[LonelyCat124]

@hiker Quick question on how the ordering of statements goes with the dependency system - if you have an assignment such as a = a + 1 does the a access on the RHS appear before the a on the LHS? I assume so (and that makes sense) but I just wanted to double check as I have a superfluous (and unreachable) if statement if so.

[LonelyCat124]

@sergisiso this is ready for a first look.
I didn't add anything to user or dev guide for now, let me know if you think I should.

I'm waiting for hiker to clarify my question before I remove the code at lines 123 in the transformation, but I expect to remove those lines as I think they're unreachable.

[sergisiso]

Good initial implementation @LonelyCat124, I am just asking if the code can be slightly refactored to be more readable and I am wondering if some logic should leave inside the next_access methods, I am happy to discuss this one.

[sergisiso]

I found this method hard to understand until I read the apply method. I think its name is too generic and it does too many things, so I suggest breaking it up (see name suggestions below).

I also notice that they all iterate over potential var_accesses and select/filter the ones passing the conditions. Could we use the python filter functionality? If the apply would be something like this:

var_accesses = VariablesAccessInfo(nodes=node.loop_body)
var_accesses = filter(self._is_local_array, var_accesses)
var_accesses = filter(self._have_same_unmodified_index, var_accesses)
var_accesses = filter(self._first_access_is_write, var_accesses)
var_accesses = filter(self._not_used_after_loop, var_accesses)

It would be very readable (so much so that I think it doesn't need inline comments) and makes each method simpler (to output array, no iteration, ...). Also the methods don't use self or node (except the last one but see next comment), should they be static methods or even just inner functions of apply?

Could you explore if this would work.

[LonelyCat124]

Its doable I think, I've moved them to be static methods (I don't like inner functions), they do still need var_accesses (and node in the final functions case), so there is a lambda in the filter calls, but it seems to work. I will need to rewrite tests for this and maybe expand the apply testing.

[sergisiso]

When you add the docstring, can you bring what you wrote in apply here and add a simple example, e.g. like src/psyclone/psyir/transformations/hoist_trans.py . So then we just need to list the autoclass under the available transformations section of doc/user_guide/transformations.rst and can be tested with python -m doctest src/psyclone/psyir/transformations/scalarization_trans.py. The apply docstring can be a oneliner then.

[sergisiso]

I was expecting the check for next accesses to only be this. Why do we need the rest, is it because the next_access implementation is currently missing these? E.g. if we use next_accesses in other places, e.g. when deciding where to place a halo_exchage to be safe, wouldn't we need the same logic as below? In this case shouldn't this all be inside the next_access implementation?

Also I see that next access only returns one reference, was this the right choice?, given that potentially is more than one:

a = 3
if condition:
    a = 1
else:
    a = 1

Also the a in the inner loop has 2 potential next_acesses

do i=1, 10
     a = 1
     do j 1=1, 10
          a = 2
     endo
enddo
a = 3

[LonelyCat124]

I deliberately kept the next_access function simple and tried to avoid the complicated cases (and rely on the dependency analysis tooling) like this, and just assume these are ok.

I can try to make a new issue and PR to improve that functionality if you think its necessary/useful. I feel like there can always be an incredibly complex structure of nested loops that makes this difficult.

[sergisiso]

But wouldn't this complex logic be for everywhere that we want to prove if a symbol is going to be accessed next or not, not just for this transformation?

[sergisiso]

I deliberately kept the next_access function simple and tried to avoid the complicated cases (and rely on the dependency analysis tooling)

I am not sure if it's next access or the VariablesAccessInfo tooling itself that needs to provide this.

@hiker and @JulienRemy I am trying to understand this. Is this related to the logic you needed on building the DAG? That the VariablesAccessInfo returns ALL following dependencies under a node (but not looking upwards - like we need in my loop snippet above) and the next_accesses just one is code order?

But what we need (and maybe next_access should be) is a method that return any "directly reachable" accesses to the same symbol, which could be more than one, and will be equivalent to the arrows leaving from a node in the DAG?

[LonelyCat124]

I think this is a separate issue (though I'm not sure about the loop case, we might need to gracefully fail somehow for some case, I'll try to remember to put a test in for that).

We otherwise ignore the if case here (which could be improved if next_acccess gave all options, either through the access info or otherwise) and return False instead of trying to work it out.

[LonelyCat124]

@sergisiso I think I could probably do most of this for next access, butt I'm not sure on if there is an easy way to tell whether a node is inside an else if or an else or an if, for example for a case like (this is how we have the if/elseif behaviour in Psyclone)

a = 1
if cond1 then
   a = 2
else
  if cond2 then
    a = 3
    a = 6
  else
    if cond3 then
       a=4
end if 
end if
end if
a = 5

We currently would find the a=2 case, and the proposal is that we need to also find the a=3, a=4, and a=5 cases. The trick is knowing that the a=3 and a=4 is inside an else if case, so to continue trying to find more "next accesses" that aren't inside the same branch of the else/if structures. We also need different behaviour for else if vs else behaviour (and all cases are complicated by the "one branch doesn't contain a next_access")

[sergisiso]

I find hard to see what each of these are checking (without going to the tests), could you add a short code snipet showing the dependency that each is looking for, inlcuding the commented one.

[LonelyCat124]

The commented one is removed because its not reachable with how the next_access/variable_access info gives results for assignments.

[LonelyCat124]

@sergisiso I've addressed most of the comments here - I think the next_access one is a bigger issue than just here (and we need to decide if the variable access tooling should handle this or if we just do it in next_access, but I think we can do an implementation for this in the mean time anyway).

I need to still expand the tests for the apply routine and add documentation before another review though I think.

[sergisiso]

@LonelyCat124 I don't mind the order of these (if you put the associated TODOs), but wouldn't want to go very far with some logic that then will need to be deleted.