Reorder block traversal order for correct hoisting.

[DrTodd13]

Resolves #9379

[DrTodd13]

Please suggest a testing strategy. The original example is quite complex and it isn't even clear how to simplify it. I think the original bug generates a race condition and too many simplifications may cause the bug not to manifest. Alternatively, we could try to look at the diagnostics and see if any build_lists are being hoisted.

[netomenoci]

Hi @DrTodd13 .
My original code was much more complex than the one shared in the reproducible, and I have simplified it the most I could. Further simplifications don't produce the error, which makes this error particularly difficult to be found. If you want , I can add a test similar to the one I have shared in the reproducible. If so, would you want me to make a PR into this branch?

[DrTodd13]

The prior code was actually okay but requires the blocks to be processed in the right order. The critical thing is that if there is a getattr on an object and then a call of that getattr, the getattr has to precede the call for the code that detects multiple writes to an object to detect this scenario as a second write. My attempt at copying the offending code into a Numba test didn't work because the block order there was one that didn't manifest the problem. Any test you could write now that used normal code I suppose you'd have no guarantee that the block order would continue to manifest the problem with any change later.

[DrTodd13]

@sklam Do you think these test failures are related to the PR? Seems unrelated to me.

[DrTodd13]

/azurepipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[DrTodd13]

/azurepipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[netodw]

@sklam , would you be able to review this PR? The issue is still there.

[thomasfrederikhoeck]

might be related to #9582 but I'm not sure to be honest.

[sklam]

Confirmed that this fixes #9379 but it has also been fixed by #9532.
This doesn't fix #9586.

[DrTodd13]

Confirmed that this fixes #9379 but it has also been fixed by #9532. This doesn't fix #9586.

So, 9532 fixes 9379 and 9586? 9397 only fixes 9379?

[sklam]

Confirmed that this fixes #9379 but it has also been fixed by #9532. This doesn't fix #9586.

So, 9532 fixes 9379 and 9586? 9397 only fixes 9379?

#9532 fixes #9379
#9532 does NOT fix #9586.
#9586 as a PR is needed for #9581, #9490.

I am trying to understand what remaining cases are fixed by this PR uniquely. The original problem in #9379 seems to come from build_tuple hiding dependency of mutable object in code like:

func([<listcomp>], *args)

where the build_tuple comes from handling of *args, which is translated to func( ( [<listcomp>] ,) + args ). I tested this by changing the original reproducer from

        res = func([data[start_idx:end_idx, ...] for data in dataargs], *funcargs) 

to

        res = func([data[start_idx:end_idx, ...] for data in dataargs]) 

and confirmed that the bug goes away.

I am unable to find a case that depends on the block traversal order. I believe such a case may appear from Numba internal changes; e.g. changes to inline-closure, changes to parfors details.

I think this patch will prevent non-deterministic bug that may be introduced from randomized block insertion from other passes or changes to internal details. Since it seems unlikely for users to create such a case, and I am slightly concerned that the reordering may expose some unforeseen bugs, I suggest we merge it for 0.61 milestone.