[PATCH] D111337: [fir] Add array value copy pass

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

@kiranchandramohan do you plan on reviewing/approving the current version of the patch?

I don't have any more comment than before, there are just a few that are still pending.



================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:423
+                                            mlir::Location loc,
+                                            mlir::Value component);
+
----------------
mehdi_amini wrote:
> This is unused in this patch?
> 
Missed this one?



================
Comment at: flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h:359
+                                  AsSequence(X)>
+#define mkRTKey(X) mkKey(RTNAME(X))
+
----------------
mehdi_amini wrote:
> Can you document all this macro system?
> Does it need to in header? In general macros defined in header is "dangerous", should these be prefixed with Fir or something?
Marked done, but I don't see a change here?


================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:355
+  auto boxTy = fir::BoxType::get(elementType);
+  return exv.match(
+      [&](const fir::ArrayBoxValue &box) -> mlir::Value {
----------------
clementval wrote:
> mehdi_amini wrote:
> > I don't quite find what the `.match(` call resolves to on the `fir::ExtendedValue` class? Any tip?
> https://github.com/llvm/llvm-project/blob/102d2a8a99057b2f54ca97661f862e33d055171a/flang/include/flang/Optimizer/Builder/BoxValue.h#L406
Thanks!

Something really confusing is that there is another class with the same name, and located in a header with the same filename! 
(in `include/flang/Lower/Support/BoxValue.h`)


================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:30
+                                      "name"),
+                       llvm::cl::init(32));
+
----------------
mehdi_amini wrote:
> I'm wary of global options in general (Can we build the optimizer with `-Werror=global-constructors` by the way?), what is this used for? Can you find an alternative (like store it on the dialect for example)?
This is marked done, but you actually extracted it as-is and landed it in https://reviews.llvm.org/D112057 ; can you address that as well (in a separate patch, but I'm wary of losing track of this).



================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:46
+                                                llvm::StringRef name) {
+  return modOp.lookupSymbol<fir::GlobalOp>(name);
+}
----------------
mehdi_amini wrote:
> Do these one-line functions carry their weight? 
> Seems like just a renaming that kind of obfuscate what's happening (I'd have to jump through more hoops in the IDE to figure it out).
Same here: you marked it done without commenting and landed it separately.


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:468
+            LLVM_DEBUG(llvm::dbgs() << " access: " << *acc << '\n');
+            if (mlir::isa<ArrayFetchOp, ArrayUpdateOp, ArrayModifyOp>(acc)) {
+              if (useMap.count(acc)) {
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > Can this check fail? Right now the contract of `arrayAccesses()` is to only return these three operations, and `conflictOnMerge` would fail if there were any other op.
> > Can this be replace with an assert here?
> You marked it done, but I don't see it done in the code.
Yet again
(line 452 by now)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111337