[PATCH] D112910: [flang] Add TargetRewrite pass

Wed Nov 3 02:16:26 PDT 2021

rovka 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;
----------------
schweitz wrote:
> 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.
I agree that it will probably be needed in the future and we can add it back when that happens. I also removed the byval, which I'll be adding back in the very next patch (rewriting complex stuff). I'd like to avoid having dead/untested code around, since that makes it difficult to know for sure what's properly supported. If somebody did need the sret for something and ran into this bug it would probably take them more time to figure out what's going on than it would take them to add it from scratch.

================
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
----------------
schweitz wrote:
> 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.
Sounds like a good idea, thanks.

================
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>
----------------
schweitz wrote:
> 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. 
> I don't think `llvm_unreachable` is what we want here. (Especially introducing undefined behavior for what could become a valid case.)

It's only undefined behaviour for release builds :) Otherwise it's a very important tool for finding and debugging issues. 

In any case, I only suggested llvm_unreachable because that's what I'm used to from the rest of llvm. What I really meant was something that would either crash or elegantly error out rather than roll along and generate something that we're uncertain about. Do we have something better that we can use? I see this has been discussed before [[ https://reviews.llvm.org/D79507 | here ]] but I don't know the whole story.

> 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. 

I'm not sure that's the best approach. If code outside TargetRewrite doesn't know how to handle a single boxchar sret it seems unlikely that it will know how to handle multiple boxchar srets, so we'll probably either get errors in other parts of the compiler or worse generate wrong code. I think it's better to be conservative and get a hard error here (and in any other places that may touch sret but don't yet handle multiple srets). This will make it easier to figure out which parts need to be updated if we do decide to support multiple srets in the future.

As a sidenote, I also tried to run a test with 2 srets, and the output seemed buggy to me (it left both srets in but also added a rewritten one). I didn't investigate much because if this is not supported in the rest of the pipeline I don't think it's worth putting any effort in it right now.

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

https://reviews.llvm.org/D112910