This is an archive of the discontinued LLVM Phabricator instance.

[flang] Preserve bound info for pointer assignments through derived types

Authored by jpenix-quic on Dec 11 2022, 1:23 PM.



Doing a pointer assignment to another pointer which is a derived type component
could result in the bound information being lost, potentially leading to
incorrect array accesses. Fix this by trying to retain the bound info during
the assignment.

Fixes #57441

Diff Detail

Event Timeline

jpenix-quic created this revision.Dec 11 2022, 1:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2022, 1:23 PM
jpenix-quic requested review of this revision.Dec 11 2022, 1:23 PM
jpenix-quic added inline comments.Dec 11 2022, 1:33 PM

I'm not sure what to do about the explicitParams field in the BoxValue constructor below. For the lbounds, grabbing the bounds from the original array and dropping them if we are taking a slice seems correct to me, but this doesn't seem right for the explicitParams/lens. Should I be trying to add the lengths of the slice or something else? I guess always dropping the info would be "safe" in the sense that that was the previous behavior, but I'm not sure what is correct/expected.


In the test here, there is a redundant shift to get the lower bounds that I think is coming from the code here ( I had some success with eliminating the shift for this test in particular by trying to look through arr via getAddr() and checking the operands for a shift/shape_shift and reusing that. However, this seemed to be a fairly brittle check (though I might have just been doing things in an odd way) and only seemed to be safe if the lbounds passed into the function was empty (indicating no bounds remapping, if I understand correctly). As such, I ended up dropping this. Should I be worried about the redundant shift in this case or is there a different/better way I could approach this?

It looks like you are doing the right thing here, thanks !


That seems a correct thing to do. Outside of when creating a MutableBox, propagating the non deferred length parameters does not matter from a semantic/correctness point of view (because the lengths are in the box anyway), but it will matter for performance at some point. For instance: character(n), contiguous :: x(:), y(:) The non deferred parameter n will allow optimizing an assignment x=y because it allows deducing that x and y are the same length and that given no padding is needed, the whole array assignment can be done as a block memcopy.

So it is better to keep track of it as you are doing here.


It does not matter. fir::ShiftOp has no side effect, so the common subexpression elimination pass will get rid of the redundant fir.shift (you can test with fir-opt-cse). While it is frustrating to produce and see redundant IR in the tests, if it is not trivial to avoid it, it is best to rely on the CSE pass to do it.

@jeanPerier Thank you for going over my questions--the help and explanations are greatly appreciated!

If I am understanding correctly, I think I can leave things as they are, so I will fix up the title/description/one source comment and then I think this should be ready for review!


Ok, makes sense and thank you for the explanation! If I am understanding correctly then, I will leave things as is!


Got it, thanks! I will leave it as is then.

jpenix-quic retitled this revision from [flang] WIP: Preserve bound info for pointer assignments through derived types to [flang] Preserve bound info for pointer assignments through derived types.
jpenix-quic edited the summary of this revision. (Show Details)

Remove old source note I left in, remove WIP and note about questions from the title and summary.

Looks good to me, thanks for finding and fixing the issue !

jeanPerier accepted this revision.Dec 12 2022, 12:35 PM
This revision is now accepted and ready to land.Dec 12 2022, 12:35 PM

Confirmed that cam4_r passes with this change. Thanks once again @jpenix-quic.