[PATCH] D116927: [flang] Add tests for converting arrays and refs to arrays. NFC

Mon Jan 10 05:47:44 PST 2022

awarzynski added a comment.

Thanks for adding these! I think that I've identified a couple more cases that are not covered here. Would it make sense to test them as well?

The conversions are not 100% clear to me TBH. For example, `!fir.ref<!fir.array<10x?x11xf32>>` becomes a plain `!llvm.ptr` (with no `array` inside) because the number of rows is unknown, correct?

================
Comment at: flang/test/Fir/types-to-llvm.fir:6

 // Test sequence types `!fir.array`

----------------
How about `fir.char`? https://github.com/llvm/llvm-project/blob/5eb47961c42cef03d832c646860a1eff6f8b4104/flang/lib/Optimizer/CodeGen/TypeConverter.h#L291

================
Comment at: flang/test/Fir/types-to-llvm.fir:32

 // Test reference types `!fir.ref`

----------------
How about `fir.box` inside `fir.ref`? https://github.com/llvm/llvm-project/blob/5eb47961c42cef03d832c646860a1eff6f8b4104/flang/lib/Optimizer/CodeGen/TypeConverter.h#L282

================
Comment at: flang/test/Fir/types-to-llvm.fir:48
+// CHECK-LABEL: foo4
+// CHECK-SAME: !llvm.ptr<array<11 x array<10 x f32>>>

----------------
There's a switch from column-major to row-major here, right? (hence `11` and `10` are swapped). Might be worth adding a comment.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116927