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

Thu Nov 4 06:30:00 PDT 2021

rovka added inline comments.

================
Comment at: flang/test/Fir/target.fir:4
+// RUN: tco --target=aarch64-unknown-linux-gnu --target-rewrite %s | FileCheck %s --check-prefix=INT64
+// RUN: tco --target=powerpc64le-unknown-linux-gnu --target-rewrite %s | FileCheck %s --check-prefix=INT64
+
----------------
mehdi_amini wrote:
> This is running a pass, this should be testable/tested with fir-opt.
> 
> If the only thing that tco provides is overriding the triple, this can also be done in a trivial pass that you just apply on the module (strawman, it'd look: `pipeline=module(overrideTargetTriple{target=aarch64-unknown-linux-gnu}, target-rewrite)`
I added a pass option instead since it seemed easier / less boilerplate.

================
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>
----------------
mehdi_amini wrote:
> schweitz wrote:
> > awarzynski wrote:
> > > 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.
> > Sorry for the misunderstanding.
> > 
> > Yes, we do not currently return multiple values. And obviously the LLVM-IR dialect isn't going to deal well with FIR types.
> > 
> > Placing a `TODO()` here is the current approach.
> > 
> > (In Flang, `llvm_unreachable` is considered bad practice precisely because it gets compiled to undefined behavior in release builds, which some developers prefer.)
> `llvm_unreachable` isn't meant for things that should work but aren't implemented yet, they are meant for things that really cannot happen from a system point of view: I should be able to plug a Fuzzer in front of a component and never hit an unreachable (assuming you don't break a documented API invariant).
> In general the invariant for the IR is "does it pass the verifier": hitting an `unreachable` on "valid IR" wouldn't seem right to me.
> (llvm::report_fatal_error provides with the same "helpful message and context" you referred to, but isn't optimized away in release mode)
Ok, thanks, TODO sounds fine to me.

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

https://reviews.llvm.org/D112910