[flang-commits] [PATCH] D142342: [flang][hlfir] Add hlfir.copy_in and hlfir.copy_out operations

[Jean Perier via Phabricator via flang-commits]

jeanPerier added inline comments.


================
Comment at: flang/include/flang/Optimizer/HLFIR/HLFIROps.td:534-537
+    This operation allows passing non contiguous arrays as procedure reference
+    dummy arguments that must be contiguous, which is possible in Fortran (that
+    mandates that any pointers made to these dummy arguments are invalidated after
+    the call).
----------------
PeteSteinfeld wrote:
> I didn't understand this sentence.
Rephrased the sentence and removed the part in parenthesis that brings little here.


================
Comment at: flang/include/flang/Optimizer/HLFIR/HLFIROps.td:560
+    OpBuilder<(ins "mlir::Value":$var, "std::optional<bool>":$handle_optional,
+      "bool":$return_box)>
+  ];
----------------
vzakhari wrote:
> Can you please document this parameter?  Is the intended usage to produce `!fir.box`, when the temporary value is "don't care" after the call, and `!fir.ref` otherwise?
I changed the operation to always return a fir.box because it made things a lot simpler, so I removed this boolean.


================
Comment at: flang/include/flang/Optimizer/HLFIR/HLFIROps.td:577
+    /// box may contain a null address.
+    static constexpr bool kHandleNullAddr = !kHandleNullBox;
+
----------------
vzakhari wrote:
> nit: it seems a two-bit enum could have been more clear, but I can accept `bool` for this purpose as well :)
Fair point, an MLIR enum would have save custom printing/parsing. I anyway removed this while simplifying the operation.


================
Comment at: flang/include/flang/Optimizer/HLFIR/HLFIROps.td:579-587
+    /// Does this operation handle a null input box?
+    bool getHandleNullBox() {
+      return getHandleOptional() && *getHandleOptional() == kHandleNullBox;
+    }
+
+    /// Does this operation handle an input box containing a null address?
+    bool getHandleNullAddr() {
----------------
PeteSteinfeld wrote:
> "getHandleNullBox()" is not used anywhere.  Do we need it?
> 
> Both of these functions would read more clearly if their names evoked the fact that they return bools.  For example, "handlesNullAddr".
> 
> I couldn't find a definition of "getHandleOptional" anywhere.
I removed those while reworking the operation. This patch is like adding am API for review, the actual usage of the API will come in a later patch.

> I couldn't find a definition of "getHandleOptional" anywhere.

MLIR generates getters automatically for all the names after "let arguments = " it convert the snake case .td name syntax (handle_optional) to camel case (getHandleOptional). See https://mlir.llvm.org/docs/DefiningDialects/Operations/#operation-arguments for more details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142342