[PATCH] D82067: [flang] addition of Coarray and some header files

[Eric Schweitz via Phabricator via llvm-commits]

schweitz marked 4 inline comments as done.
schweitz added a comment.

In D82067#2101516 <https://reviews.llvm.org/D82067#2101516>, @kiranchandramohan wrote:

> I have a few comments/questions.
>
> Could this have been better as more than one patch? Box, symbolmap in one, RTBuilder in another, and corray in another?


Indeed, I really did consider that. With this one, I just went with increasing the scope a bit to include four smaller support files that will be need to be merged to start upstreaming future modules. It is a change of pace from the eyedropper approach.

Trying to figure out what size patch is too big or too small is always a problem. If this patch must be broken down into 4 or 5 much smaller pieces, it can be abandoned.



================
Comment at: flang/include/flang/Lower/Support/BoxValue.h:176-177
+
+/// Used for triple notation (array slices)
+using RangeBoxValue = std::tuple<mlir::Value, mlir::Value, mlir::Value>;
+
----------------
kiranchandramohan wrote:
> RangeBoxValue is not used.
> Also, is the name a misnomer since it has nothing to do with the abstract box classes?
Nice spot.

It will be used. It's included now rather than create fussy merge conflicts.



================
Comment at: flang/lib/Lower/Coarray.cpp:15-16
+#include "flang/Lower/Coarray.h"
+#include "RTBuilder.h"
+#include "SymbolMap.h"
+#include "flang/Lower/Bridge.h"
----------------
kiranchandramohan wrote:
> Are these two headers needed here?
Technically, no.

On the other hand, it's hard to imagine how one might lower a coarray without access to symbols or calls to runtime libraries with runtime library building code.



================
Comment at: flang/lib/Lower/RTBuilder.h:28
+// List the runtime headers we want to be able to dissect
+#include "../../runtime/io-api.h"
+
----------------
kiranchandramohan wrote:
> should this header be in the include directory?
It's only germane to lowering itself, so it is better not to expose it.


================
Comment at: flang/lib/Lower/SymbolMap.h:129
+        common::visitors{[&](const FullDim &box) {
+                           assert(dim < box.lbounds.size());
+                           return box.lbounds[dim];
----------------
kiranchandramohan wrote:
> https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/include/llvm/ADT/SmallVector.h#L149
> 
> Isn't this assert part of the SmallVector implementation?
Thanks. I'll get rid of these.


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

https://reviews.llvm.org/D82067