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

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


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


================
Comment at: flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h:359
+                                  AsSequence(X)>
+#define mkRTKey(X) mkKey(RTNAME(X))
+
----------------
mehdi_amini wrote:
> 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?
No idea why it was mark done since I don't even remember reading this one. I have added some comments and prefixed the macros. I kept `mkRTKey` without prefix since it is used outside. 


================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:30
+                                      "name"),
+                       llvm::cl::init(32));
+
----------------
mehdi_amini wrote:
> 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).
> 
Sorry, I marked it as done here to be able to navigate more easily in the comments since it was not part of this part anymore. I'll look at it in another patch. 


================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:46
+                                                llvm::StringRef name) {
+  return modOp.lookupSymbol<fir::GlobalOp>(name);
+}
----------------
mehdi_amini wrote:
> 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.
Is this really important? If so I'll update it. 


================
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:
> > 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)
My bad. I'm sure I made the change but I probably mixed up my branches and it was not in this update. Updated now. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111337