[PATCH] D112910: [flang] Add TargetRewrite pass

[Diana Picus via Phabricator via llvm-commits]

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


================
Comment at: flang/lib/Optimizer/CodeGen/TargetRewrite.cpp:25-29
+//===----------------------------------------------------------------------===//
+// Target rewrite: rewriting of ops to make target-specific lowerings manifest.
+// LLVM expects different lowering idioms to be used for distinct target
+// triples. These distinctions are handled by this pass.
+//===----------------------------------------------------------------------===//
----------------
awarzynski wrote:
> [nit] Why not move this to the top of the file? It looks like a comment that will apply to everything in this file.
Fair point


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


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

https://reviews.llvm.org/D112910