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

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski accepted this revision.
awarzynski added a comment.
This revision is now accepted and ready to land.

LGTM!

Thank you for addressing my comments :)



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


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

https://reviews.llvm.org/D112910