Page MenuHomePhabricator

[flang] Update Reshape runtime library routine and its corresponding unit test
ClosedPublic

Authored by mleair on Jun 18 2021, 7:03 PM.

Details

Summary

Update Reshape so it uses a result argument instead of a result return object. The result argument is consistent with other intrinsics and more in line with the
code generated by fir lowering.

Also, updated reshape.cpp unit test to use the new Reshape interface.

Diff Detail

Event Timeline

mleair created this revision.Jun 18 2021, 7:03 PM
mleair requested review of this revision.Jun 18 2021, 7:03 PM
mleair edited subscribers, added: flang-commits; removed: jdoerfert.Jun 18 2021, 7:10 PM
mleair updated this revision to Diff 353158.Jun 18 2021, 7:23 PM

incorporate lint comments.

mleair updated this revision to Diff 353170.Jun 18 2021, 11:43 PM

Remove unneeded check on pad.

mleair updated this revision to Diff 353177.Jun 19 2021, 12:53 AM

fix address calculation for order argument.

Looks good to me.

flang/runtime/transformational.cpp
400–402

I think auto k{GetInt64(shape.Element<char>(&orderSubscript), orderElementBytes, terminator)} might be more idiomatic than manually incrementing the byte offset. @klausler, is this correct ?

All builds, tests, and looks good to me. But I'm not very familiar with the front end's interface to the runtime.

klausler added inline comments.Jun 22 2021, 10:44 AM
flang/runtime/transformational.cpp
400–402

Yes. When something in the runtime can be written as an application of subscripts to an array, that's usually the right way to do it.

klausler added inline comments.Jun 22 2021, 10:46 AM
flang/runtime/transformational.cpp
355

needs extern "C" to match API in header

400–401

braces, please

Looks good to me.

flang/runtime/transformational.cpp
355

Not sure what you mean. There is an extern "C" that we now share in this file above (before the Cshift runtime routine).

400–402

Unfotunately, I found a case where I was getting a bad value for k when I used the original code. That is why I rewrote it. I will try it this time with "orderElementBytes" as the second argument.

400–402

Good news. I got it to work, but after using orderElementBytes and order->Element instead of shape.Element. So, the following line appears to work:

auto k{GetInt64(order->Element<char>(&orderSubscript), orderElementBytes, terminator)};

I will make this change and update the review.

mleair updated this revision to Diff 353841.EditedJun 22 2021, 7:05 PM

Incorporate comments from previous diff.

I also added an "order" argument to the reshape unit test to exercise order element calculation. I hope it's OK that the test reports 88 tests pass instead of 86. If that's a problem, I can remove the 2 extra tests.

@klausler @jeanPerier @PeteSteinfeld Do you have any more comments?

klausler accepted this revision.Jun 24 2021, 9:28 AM
This revision is now accepted and ready to land.Jun 24 2021, 9:28 AM
This revision was landed with ongoing or failed builds.Jun 24 2021, 5:06 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2021, 5:06 PM