(closes #2884) Allow WHERE with imported symbols by changing Ref2ArrayRange behaviour to do the validations #3260
Conversation
sergisiso
temporarily deployed
to
integration
GitHub Actions
Inactive
sergisiso
temporarily deployed
to
integration
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3260 +/- ##
==========================================
- Coverage 99.91% 99.91% -0.01%
==========================================
Files 376 376
Lines 53529 53522 -7
==========================================
- Hits 53484 53475 -9
- Misses 45 47 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sergisiso
temporarily deployed
to
integration
GitHub Actions
Inactive
sergisiso
temporarily deployed
to
integration
GitHub Actions
Inactive
sergisiso
changed the title
|
@arporter @LonelyCat124 This is ready for review. It changes the Reference2ArrayReference to fail if the outcome cannot be guaranteed, so thing like WHERE can rely on its validation. This will be followed by #1858 (structures) and #2722 (dependencies), which can be done in a single location after this PR, but it was to big to do here. |
arporter
left a comment
arporter
left a comment
There was a problem hiding this comment.
Really nice Sergi, it's good to see the simplification that this has achieved.
I'm worried that we are no longer raising an exception when we see a StructureReference - given that we can't handle them this needs to be flagged?
Apart from that, it's just minor tidying. I'll run the integration tests next time (or you could fire them off perhaps once you're done).
| an Array) to a PSyIR Range. For example: | ||
| ''' | ||
| Transformation to convert plain References of array symbols to | ||
| ArrayReferances with full-extend ranges if it is semantically equivalent |
| to do so (e.g. it won't convert call arguments because it would change the | ||
| bounds values). | ||
| Note that if a node that does not need to be modified is provided (e.g. |
There was a problem hiding this comment.
"...if the provided node does not need..."
| # TODO issue #1858. Add support for structures containing arrays. | ||
| if node and node.parent and isinstance(node.parent, Call): | ||
| if node.position == 0: | ||
| return |
There was a problem hiding this comment.
Comment please - I didn't readily understand at first but presumably this is checking that the node isn't the Reference associated with the routine target of a Call?
There was a problem hiding this comment.
Exactly. Added comment
| if node.parent.is_elemental is None: | ||
| raise TransformationError(LazyString( | ||
| lambda: f"The supplied node is passed as an argument to a " | ||
| f"Call that we don't know if it is elemental or not: " |
There was a problem hiding this comment.
"...that may or may not be elemental: ...."
| return | ||
|
|
||
| if ( | ||
| type(node) is ArrayReference or |
There was a problem hiding this comment.
This probably needs a pylint disable for unidiomatic type check?
| :type symbols: List[str] | ||
| :param scalars: list of symbol names that must be added to the symbol | ||
| table with an scalar datatype. | ||
| :type symbols: List[str] |
There was a problem hiding this comment.
Please replace with typehints.
| with pytest.raises(TransformationError) as info: | ||
| trans.validate(Reference(Symbol("x"))) | ||
| assert ("The supplied node should be a Reference to a symbol of known " | ||
| "type, but 'x' is not." in str(info.value)) |
There was a problem hiding this comment.
It might be useful to include str(sym) in the error message so that we can see what PSyclone thinks 'x' is.
| assert ("The supplied node should be a Reference but found " | ||
| "'ArrayReference'." in str(info.value)) | ||
|
|
||
| trans.validate(None) |
There was a problem hiding this comment.
Couldn't you have this as the first step in the previous test (i.e. before "If it is not even a Reference")?
There was a problem hiding this comment.
Oh yes, it is already there, removing this one
| trans.validate(reference) | ||
| assert ("node is passed as an argument to a Call to a non-elemental " | ||
| "routine (DEALLOCATE(a)" in str(info.value)) | ||
| trans.validate(assign.lhs) |
There was a problem hiding this comment.
The docstring says we test that a StructureReference raises an exception but clearly it doesn't any longer. Given that we don't support them yet, I would argue we should raise an exception as we don't know what to do with them.
There was a problem hiding this comment.
It felt bad to have the comment that are not supported and then just return the validation.
The reason I did it is because both the WHERE and the ArrayAssignment2Loop already implement and have several tests for structure references (which they happily take at face-value, without checking if they need to be added ranges).
Adding this restriction here, without implementing proper support for letting valid cases pass, and array types extended, makes all of those fail.
Also this is not worse than master, in master it had an exception but it was "except:pass" everywhere it was used.
I suggest I tackle this in a follow up (I attempted starting it but it quickly grows very large), or we can block this for now. Let me know what you prefer.
| trans.validate(assign.lhs) | ||
|
|
||
|
|
||
| def test_validate_pointer_assignment(fortran_reader): |
There was a problem hiding this comment.
We probably don't need this test anymore?
There was a problem hiding this comment.
I added the check back (it was important for NEMOv4 performance because tmask is a pointer) and the check is more precise. So I kept (and extended the test).
sergisiso
temporarily deployed
to
integration
GitHub Actions
Inactive
sergisiso
force-pushed
the
2884_change_ref2arrayrange_behaviour
branch
from
99ab3e0 to
3e1c6b8
Compare
sergisiso
temporarily deployed
to
integration
GitHub Actions
Inactive
|
@arporter This is ready for another look, I just fired the integration test. |
sergisiso
temporarily deployed
to
integration
GitHub Actions
Inactive
Change Ref2ArrayRange behaviour by letting it pass if the provided node is already in the expanded form, but making it fail if for a expression we cannot guarantee if it could be expanded or not. This will bring validations that now need to be done every time its used to inside the transformation.