[PATCH] D114241: [fir] Add !fir.len_param_index conversion

[Eric Schweitz via Phabricator via llvm-commits]

schweitz 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);
----------------
jeanPerier wrote:
> 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 ?
LEN (variable) type parameters may be stored in the descriptor of an object or elsewhere.

In the FIR model, they are a special set of integral members in an instance of a derived type. They behave like ordinary data members and can be addressed and accessed.

The reason for the FIXME is that lowering will require more infrastructure work before it can correctly access the requested variable type parameter.


================
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>
+
----------------
jeanPerier wrote:
> 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.
It should _not_ accept field names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114241