[flang-commits] [PATCH] D114241: [fir] Add !fir.len_param_index conversion

[Jean Perier via Phabricator via flang-commits]

jeanPerier added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:852
+    // add the offset into the LENs
+    offset += 0; // FIXME
+    auto attr = rewriter.getI64IntegerAttr(offset);
----------------
rovka wrote:
> I'm not entirely sure what we're supposed to compute here. My understanding is that we have a CFI_cdesc_t with an addendum right after it, and the addendum looks [[ https://github.com/llvm/llvm-project/blob/6623c02d70c3732dbea59c6d79c69501baf9627b/flang/include/flang/Runtime/descriptor.h#L84 | like DescriptorAddendum ]].
> So the offset we're trying to compute needs to skip over the whole CFI_cdesc_t and then the derivedType_ member, and potentially other LEN parameters that come first in len_. 
> Is this correct? 
> Also, what's [[ https://github.com/flang-compiler/f18-llvm-project/blob/1e39e4822cbe2fedbc5dc269ade9f430287a0eda/flang/lib/Optimizer/CodeGen/TypeConverter.h#L213 | this TODO ]] about?
> I'm not entirely sure what we're supposed to compute here.

Not an expert on this op (this is not used yet in lowering since it deals with >= F2003 features). It looks like this is computing the offset in bytes (from the descriptor base address) where the length parameter lies in memory.

The TODO is related to the fact that its current codegen always returns the first length parameter offset, instead of actually looking for the one that is queried, so it is clearly wrong and would probably deserve a hard TODO.

The FIROps.td description tells this op can be combined with a fir.coordinate_of to retrieve the length parameters address.

It looks very early design to me, and we should probably re-think the approach here, I am not sure we need an op that gives us the length parameters address, I think we will most likely always want to query the value from a fir.box. The length parameters will most likely be written into the descriptor by the runtime / fir.embox or fir.rebox. But this is design that need to be discuss first.

So I am not sure what to say here, maybe a hard TODO in the codegen part is the way to go for now ?


================
Comment at: flang/test/Fir/convert-to-llvm-target.fir:162
+  %5 = fir.len_param_index l2, !fir.type<twolens>
+  %6 = fir.len_param_index l, !fir.type<twolens>
+
----------------
rovka wrote:
> Why does it work to use the names of fields? I would expect it to blow up if we pass something that isn't a len param.
I agree the verifier should complain, it makes no sense to not use a len parameter name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114241