[flang-commits] [PATCH] D116926: [flang] Fix overallocation by fir-to-llvm-ir pass

[Diana Picus via Phabricator via flang-commits]

rovka added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:376
       if (auto seqTy = allocEleTy.dyn_cast<fir::SequenceType>()) {
-        fir::SequenceType::Extent constSize = 1;
-        for (auto extent : seqTy.getShape())
-          if (extent != fir::SequenceType::getUnknownExtent())
-            constSize *= extent;
-        mlir::Value constVal{
-            genConstantIndex(loc, ity, rewriter, constSize).getResult()};
-        size = rewriter.create<mlir::LLVM::MulOp>(loc, ity, size, constVal);
+        if (!seqTy.hasConstantInterior()) {
+          fir::SequenceType::Extent constSize = 1;
----------------
schweitz wrote:
> This does not appear to be the correct.
> 
> ```
>    alloca !fir.array<*:i32>, %1, %2
> ```
> has a constant sized interior (the element type i32 has a constant size), but it is clearly not valid to drop the array shape arguments.
> 
> This applies to a case like
> ```
>    alloca !fir.array<5x6x?x?xf64>, %3, %4, %5
> ```
> as well. In this second case, space of `%3 * %4 * %5 * 5 * 6 * sizeof(f64)` bytes are required to be allocated even though the type has a constant interior of `5 * 6 * sizeof(f64)`.
> 
If I understand correctly, the first example is not valid, since you [[ https://github.com/llvm/llvm-project/blob/3012f35f8727408550d5308a1c28a66cd005a852/flang/lib/Optimizer/Dialect/FIROps.cpp#L46 | can't call alloca on a sequence type with unknown rank ]]. You'd get an error from [[ https://github.com/llvm/llvm-project/blob/3012f35f8727408550d5308a1c28a66cd005a852/flang/lib/Optimize Am I missing something?r/Dialect/FIROps.cpp#L242 | here ]]. 

As for the second example, it's very similar to the test I'm adding at line 1119, alloca_const_interior_array. That has
```
fir.alloca !fir.array<8x9x?x?xf32>, %0, %1
```
and it allocates `%0 * %1 * array<9 x array<8 x f32>>`, so I believe it's allocating the right amount. Am I missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116926