[flang-commits] [PATCH] D112910: [flang] Add TargetRewrite pass

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski added a comment.

Thanks for addressing my comments! I've added a few more suggestions, but nothing major.



================
Comment at: flang/include/flang/Optimizer/CodeGen/CodeGen.h:31
+/// Prerequiste pass for code gen. Perform intermediate rewrites to tailor the
+/// IR for the chosen target.
+std::unique_ptr<mlir::OperationPass<mlir::ModuleOp>> createFirTargetRewritePass(
----------------
[nit] In line with the change in TargetRewrite.cpp, IR --> FIR


================
Comment at: flang/lib/Optimizer/CodeGen/Target.cpp:38
+    // NB: Other conventions/ABIs can/should be supported via options.
+    marshal.emplace_back(idxTy, AT{0, {}, {}, /*append=*/!sret});
+    return marshal;
----------------
I think that I understand why `append=!sret`, but why is `sret` initialized with `{}`rather than the input argument?


================
Comment at: flang/lib/Optimizer/CodeGen/Target.h:70
+
+  /// Type presentation of a `boxchar<n>` type argument when passed by value. An
+  /// argument value may need to be passed as a (safe) reference argument.
----------------
nit


================
Comment at: flang/lib/Optimizer/CodeGen/Target.h:74
+  /// A function that returns a `boxchar<n>` type value must already have
+  /// converted that return value to an sret argument. This requirement is in
+  /// keeping with Fortran semantics, which require the caller to allocate the
----------------
rovka wrote:
> awarzynski wrote:
> > What is `sret`?
> It's an LLVM [[ https://llvm.org/docs/LangRef.html#id1434 | parameter attribute  ]] for a parameter that's actually the return value of the function (probably short for 'struct return'). Would it be clearer if I rephrased the comment to "[...] converted that return value to an argument with the sret attribute"? The attribute is also mentioned in the last sentence in the comment, so I'm not sure if rephrasing really adds anything. 
Ah, thanks! I think that context is key here :) How about:

```
  /// A function that returns a `boxchar<n>` type value must already have
  /// converted that return value to an argument decorated with LLVM's `sret` attribute.
```
?

I don't know what `Attribute` is L78 referring to TBH. I'm guessing `fir::details::Attributes`?


================
Comment at: flang/lib/Optimizer/CodeGen/TargetRewrite.cpp:25-29
+//===----------------------------------------------------------------------===//
+// Target rewrite: rewriting of ops to make target-specific lowerings manifest.
+// LLVM expects different lowering idioms to be used for distinct target
+// triples. These distinctions are handled by this pass.
+//===----------------------------------------------------------------------===//
----------------
[nit] Why not move this to the top of the file? It looks like a comment that will apply to everything in this file.


================
Comment at: flang/test/Fir/target.fir:33
+// INT64-SAME: [[ARG0:%[0-9A-Za-z]+]]: !fir.ref<!fir.char<1,?>> {llvm.sret}, [[ARG1:%[0-9A-Za-z]+]]: i64, [[ARG2:%[0-9A-Za-z]+]]: !fir.ref<!fir.char<1,?>>, [[ARG3:%[0-9A-Za-z]+]]: i64)
+func @boxcharsret(%arg0 : !fir.boxchar<1> {llvm.sret}, %arg1 : !fir.boxchar<1>) {
+  // INT32-DAG: [[B0:%[0-9]+]] = fir.emboxchar [[ARG0]], [[ARG1]] : (!fir.ref<!fir.char<1,?>>, i32) -> !fir.boxchar<1>
----------------
Would it make sense to add a few more cases here?
```
// More than one `sret` arg (verify that the array length is stored next to the `sret` arg)
func @boxcharsret_1(%arg0 : !fir.boxchar<1> {llvm.sret}, %arg1 : !fir.boxchar<1> {llvm.sret}, %arg2 : !fir.boxchar<1>) {
  return
}

// Multiple non `sret` args (verify that the array length is stored at the end, after the array args)
func @boxcharsret_2(%arg0 : !fir.boxchar<1> {llvm.sret}, %arg1 : !fir.boxchar<1>, %arg2 : !fir.boxchar<1>) {
  return
}
```

You can skip the function body and just verify that the signature is updated.


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

https://reviews.llvm.org/D112910