[PATCH] D112910: [flang] Add TargetRewrite pass

[Eric Schweitz via Phabricator via llvm-commits]

schweitz added inline comments.


================
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;
----------------
rovka wrote:
> awarzynski wrote:
> > I think that I understand why `append=!sret`, but why is `sret` initialized with `{}`rather than the input argument?
> Good catch. That looks like an oversight and it seems we're never actually checking the value of that 'sret' anyway. I'm just going to trim this class and we can add the other bits back when actually needed.
sret is a parameter to the function as well as a field in the attribute set.

Other targets and data types will need sret support. I don't know that dropping it on the floor will help.


================
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
----------------
awarzynski wrote:
> 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`?
How about putting a link to the LLVM language reference? No need to restate the LLVM concept again here, as these rewrites are specifically targeting LLVM.


================
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>
----------------
awarzynski wrote:
> rovka wrote:
> > awarzynski wrote:
> > > 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.
> > Sure, I can add a testcase for the latter, but I think if we try to do multiple srets we'll hit the FIXME in this patch at TargetRewrite.cpp line 126. To be honest I'm not sure if multiple srets are possible, my understanding of LLVM IR is that if a function has multiple results they all get bundled up in a single struct and that's the sret argument (there's no use in having more than 1). Personally I would sooner have an llvm_unreachable on that path. But then again my understanding might be blatantly wrong or skewed towards how clang does things. It would be nice to have some input from someone that actually knows FIR :)
> Thanks! I would be in favor of `llvm_unreachable` on L126 in TargetRewrite.cpp, but will leave it as a nit.
I don't think `llvm_unreachable` is what we want here. (Especially introducing undefined behavior for what could become a valid case.)

FIR allows specialization of functions and letting them return anonymous tuples/vectors of results.

That said, the definition for exactly how the ABI should look in LLVM IR for these has not been nailed down. Currently this takes a hands off approach and omits the rewriting. That may be sufficient or not in the long run. 


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

https://reviews.llvm.org/D112910