[PATCH] D112910: [flang] Add TargetRewrite pass

Wed Nov 3 04:46:24 PDT 2021

awarzynski 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:
> 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.
+1

================
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>
----------------
rovka wrote:
> 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.
>  I only suggested llvm_unreachable because that's what I'm used to from the rest of llvm. 

+1. `llvm_unreachable` is a well known and popular mechanism within LLVM. In this case, we would be using it to document that a particular case is not supported _yet_. If we do decide to  implement support for more `sret` arguments, then the compiler will hit this code and `llvm_unreachable` will contain a helpful message that will add some useful context. This context is quite clear just now (we are discussing it), but it might not be so obvious to our future selves. Or to some other developer that will be adding this support.

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

https://reviews.llvm.org/D112910