[flang-commits] [PATCH] D104586: [flang] Update Reshape runtime library routine and its corresponding unit test

[Mark LeAir via Phabricator via flang-commits]

mleair added a comment.

In D104586#2829885 <https://reviews.llvm.org/D104586#2829885>, @jeanPerier wrote:

> Looks good to me.





================
Comment at: flang/runtime/transformational.cpp:355
 // F2018 16.9.163
-OwningPtr<Descriptor> RTNAME(Reshape)(const Descriptor &source,
+void RTNAME(Reshape)(Descriptor &result, const Descriptor &source,
     const Descriptor &shape, const Descriptor *pad, const Descriptor *order,
----------------
klausler wrote:
> needs extern "C" to match API in header
Not sure what you mean. There is an extern "C" that we now share in this file above (before the Cshift runtime routine).



================
Comment at: flang/runtime/transformational.cpp:397-401
+    auto *addr = order->OffsetElement<char>(orderSubscript);
+    std::size_t orderElementBytes{order->ElementBytes()};
+    for (SubscriptValue j{0}; j < resultRank; ++j) {
+      auto k{GetInt64(addr, 1, terminator)};
+      addr += orderElementBytes;
----------------
klausler wrote:
> klausler wrote:
> > jeanPerier wrote:
> > > 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 ?
> > 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.
> braces, please
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.


================
Comment at: flang/runtime/transformational.cpp:397-401
+    auto *addr = order->OffsetElement<char>(orderSubscript);
+    std::size_t orderElementBytes{order->ElementBytes()};
+    for (SubscriptValue j{0}; j < resultRank; ++j) {
+      auto k{GetInt64(addr, 1, terminator)};
+      addr += orderElementBytes;
----------------
mleair wrote:
> klausler wrote:
> > klausler wrote:
> > > jeanPerier wrote:
> > > > 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 ?
> > > 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.
> > braces, please
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104586/new/

https://reviews.llvm.org/D104586