This is an archive of the discontinued LLVM Phabricator instance.

[flang] Fix AllocaOp/AllocMemOp type conversion
ClosedPublic

Authored by peixin on May 2 2022, 5:53 AM.

Details

Summary

For arrays without a constant interior or arrays of character with
dynamic length arrays, the data types are converted to a pointer to the
element type, so the scale size of the constant extents needs to be
counted. The previous AllocaOp conversion does not consider the arrays
of character with dynamic length arrays, and the previous AllocMemOp
conversion does not consider arrays without a constant interior. This
fixes them and refactors the code so that it can be shared. Also add
the test cases.

Diff Detail

Event Timeline

peixin created this revision.May 2 2022, 5:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 5:53 AM
peixin requested review of this revision.May 2 2022, 5:53 AM

Thanks for the bug fix

flang/lib/Optimizer/CodeGen/CodeGen.cpp
382

We should really try to share this part of the code with fir.allocmem codegen (that may or may not contain this bug, but that is doing very similar logic here: https://github.com/llvm/llvm-project/blob/44e58509be72b49c3abc7b4c5a7ffa81a754fe1c/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L1006). Can you try to share it (a lot more can probably be shared between allocmem and alloca codegen) ?

peixin updated this revision to Diff 427368.May 5 2022, 9:57 AM
peixin retitled this revision from [flang] Fix AllocaOp conversion with character array type to [flang] Fix AllocaOp/AllocMemOp type conversion.
peixin edited the summary of this revision. (Show Details)

Thanks @jeanPerier for the notice. The AllocMemOp does have one similar bug. Also fix it and share the code with AllocaOp.

jeanPerier accepted this revision.May 6 2022, 1:25 AM

Thanks a lot @peixin, it is great to have this code shared and to have more regression tests here !

This revision is now accepted and ready to land.May 6 2022, 1:25 AM
peixin added a comment.EditedMay 6 2022, 1:35 AM

Thanks @jeanPerier . FIR upstream work is not done. So, do I need to cherry-pick and sync this, D124345, and D125022 to fir-dev after landing them considering it may affect the upstream work?

This revision was automatically updated to reflect the committed changes.