[fir] Add !fir.len_param_index conversion

[rovka]

Summary

Convert len_param_index into a constant. The patch contains a lot of
FIXMEs and very likely computes the wrong value. I have a few questions
about the whole thing but I'll add them as comments so it's easier to follow
the discussion.

This patch is part of the upstreaming effort from fir-dev.

Co-authored-by: Eric Schweitz <[e-mail]>

[rovka]

flang/lib/Optimizer/CodeGen/CodeGen.cpp

852

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 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 this TODO about?

flang/test/Fir/convert-to-llvm-target.fir

162 ↗

(On Diff #388482)

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.

[jeanPerier]

flang/lib/Optimizer/CodeGen/CodeGen.cpp

852

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 ?

flang/test/Fir/convert-to-llvm-target.fir

162 ↗

(On Diff #388482)

I agree the verifier should complain, it makes no sense to not use a len parameter name.

[schweitz]

flang/lib/Optimizer/CodeGen/CodeGen.cpp

852

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.

flang/test/Fir/convert-to-llvm-target.fir

162 ↗

(On Diff #388482)

It should _not_ accept field names.

[awarzynski]

It sounds like this Op is incomplete/incorrect and not needed. I would be in favor of turning this into a TODO - otherwise it's just too confusing. But it will only makes sense if we do the same on fir-dev. WDYT?

flang/test/Fir/convert-to-llvm-target.fir

162 ↗

(On Diff #388482)

So the example in FIROps.td is incorrect, right? https://github.com/llvm/llvm-project/blob/d92aabc33666e83612c93e7c9c5c454510ba9b07/flang/include/flang/Optimizer/Dialect/FIROps.td#L2002-L2004

[rovka]

In D114241#3145739, @awarzynski wrote:

It sounds like this Op is incomplete/incorrect and not needed. I would be in favor of turning this into a TODO - otherwise it's just too confusing. But it will only makes sense if we do the same on fir-dev. WDYT?

SGTM.

I'll send a separate patch for the verifier/docs.

[rovka]

Bail out until we finalize the design.

[rovka]

flang/test/Fir/convert-to-llvm-target.fir

162 ↗

(On Diff #388482)

Actually I think the example is correct, that looks like an empty struct with just a LEN param. LEN params are in parentheses, fields are between braces.

[awarzynski]

Thanks, LGTM! Could you wait for @schweitz or @jeanPerier to have a look before landing this?

flang/test/Fir/convert-to-llvm-target.fir

162 ↗

(On Diff #388482)

Oh, that's a very fine difference, thanks for pointing this out :)

[jeanPerier]

LGTM, thanks

flang/lib/Optimizer/CodeGen/CodeGen.cpp

struct GlobalLenOpConversion : public FIROpConversion<fir::GlobalLenOp> {
   mlir::LogicalResult
   matchAndRewrite(fir::GlobalLenOp globalLen, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
     return rewriter.notifyMatchFailure(
         globalLen, "fir.global_len codegen is not implemented yet");
   }
 };
 
+/// Lower fir.len_param_index
+struct LenParamIndexOpConversion
+    : public FIROpConversion<fir::LenParamIndexOp> {
+  using FIROpConversion::FIROpConversion;
+
+  // FIXME: this should be specialized by the runtime target
+  mlir::LogicalResult
+  matchAndRewrite(fir::LenParamIndexOp lenp, OpAdaptor,
+                  mlir::ConversionPatternRewriter &rewriter) const override {
+    return rewriter.notifyMatchFailure(
+        lenp, "fir.len_param_index codegen is not implemented yet");
+  }
+};
+
 /// Lower `fir.gentypedesc` to a global constant.
 struct GenTypeDescOpConversion : public FIROpConversion<fir::GenTypeDescOp> {
   using FIROpConversion::FIROpConversion;
 
   mlir::LogicalResult
   matchAndRewrite(fir::GenTypeDescOp gentypedesc, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
     return rewriter.notifyMatchFailure(
         gentypedesc, "fir.fir.gentypedesc codegen is not implemented yet");
   }
 };
 
 /// Convert `fir.end`
 struct FirEndOpConversion : public FIROpConversion<fir::FirEndOp> {
   using FIROpConversion::FIROpConversion;
 
   mlir::LogicalResult
pattern.insert<
         BoxIsArrayOpConversion, BoxIsPtrOpConversion, BoxProcHostOpConversion,
         BoxRankOpConversion, BoxTypeDescOpConversion, CallOpConversion,
         CmpcOpConversion, ConstcOpConversion, ConvertOpConversion,
         DispatchOpConversion, DispatchTableOpConversion, DTEntryOpConversion,
         DivcOpConversion, EmboxOpConversion, EmboxCharOpConversion,
         EmboxProcOpConversion, ExtractValueOpConversion, FieldIndexOpConversion,
         FirEndOpConversion, HasValueOpConversion, GenTypeDescOpConversion,
         GlobalLenOpConversion, GlobalOpConversion, InsertOnRangeOpConversion,
-        InsertValueOpConversion, IsPresentOpConversion, LoadOpConversion,
-        NegcOpConversion, NoReassocOpConversion, MulcOpConversion,
-        SelectCaseOpConversion, SelectOpConversion, SelectRankOpConversion,
-        SelectTypeOpConversion, ShapeOpConversion, ShapeShiftOpConversion,
-        ShiftOpConversion, SliceOpConversion, StoreOpConversion,
-        StringLitOpConversion, SubcOpConversion, UnboxCharOpConversion,
-        UnboxProcOpConversion, UndefOpConversion, UnreachableOpConversion,
-        ZeroOpConversion>(typeConverter);
+        InsertValueOpConversion, IsPresentOpConversion,
+        LenParamIndexOpConversion, LoadOpConversion, NegcOpConversion,
+        NoReassocOpConversion, MulcOpConversion, SelectCaseOpConversion,
+        SelectOpConversion, SelectRankOpConversion, SelectTypeOpConversion,
+        ShapeOpConversion, ShapeShiftOpConversion, ShiftOpConversion,
+        SliceOpConversion, StoreOpConversion, StringLitOpConversion,
+        SubcOpConversion, UnboxCharOpConversion, UnboxProcOpConversion,
+        UndefOpConversion, UnreachableOpConversion, ZeroOpConversion>(
+        typeConverter);
     mlir::populateStdToLLVMConversionPatterns(typeConverter, pattern);
     mlir::arith::populateArithmeticToLLVMConversionPatterns(typeConverter,
                                                             pattern);
     mlir::ConversionTarget target{*context};
     target.addLegalDialect<mlir::LLVM::LLVMDialect>();
 
     // required NOPs for applying a full conversion
     target.addLegalOp<mlir::ModuleOp>();

flang/test/Fir/convert-to-llvm-invalid.fir

fir.global @global_derived : !fir.type<minez(f:i32)> {
   // expected-error@+1{{failed to legalize operation 'fir.global_len'}}
   fir.global_len f, 1 : i32
   %0 = fir.undefined !fir.type<minez>
   fir.has_value %0 : !fir.type<minez>
 }
 
 // -----
 
+// Test `fir.len_param_index` conversion failure. Not implemented yet.
+
+func @lenparamindex() {
+  // expected-error@+1{{failed to legalize operation 'fir.len_param_index'}}
+  %0 = fir.len_param_index l1, !fir.type<twolens(l1:i32, l2:i32){i:i32, f:f32, l:i64}>
+  return
+}
+
+// -----
+
 // Test `fir.gentypedesc` conversion failure. Not implemented yet.
 
 func @gentypedesc() {
   // expected-error@+1{{failed to legalize operation 'fir.gentypedesc'}}
   %0 = fir.gentypedesc !fir.type<derived3>
   return
 }