[flang][codegen] Add `fir.end` conversion placeholder

[awarzynski]

Summary

This patch extends the FIRToLLVMLowering pass in Flang by adding a
hook to transform fir.end. This is just a placeholder for now as fir.end is not required yet.

This is part of the upstreaming effort from the fir-dev branch in [1].

[1] https://github.com/flang-compiler/f18-llvm-project

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

[awarzynski]

Without the change in "FIROps.td" I was getting the following error:

error: custom op 'fir.end' has no custom assembly form

I'm not that familiar with the design, so perhaps this is not the right approach?

[mehdi_amini]

In D113295#3112136, @awarzynski wrote:

Without the change in "FIROps.td" I was getting the following error:

error: custom op 'fir.end' has no custom assembly form

I'm not that familiar with the design, so perhaps this is not the right approach?

flang/include/flang/Optimizer/Dialect/FIROps.td

812 ↗

(On Diff #385108)

You need this to be able to parse the "pretty" form. But do you need one?

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

331 ↗

(On Diff #385108)

You could test it with just "fir.end"() : () -> () and not define a custom parser.

[mehdi_amini]

flang/include/flang/Optimizer/Dialect/FIROps.td

812 ↗

(On Diff #385108)

If you ever need a trivial custom form in the future, the preferred way would be to write it as let assemblyFormat = "attr-dict-with-keyword";

The assemblyFormat makes sure that you don't forget elements, like the optional dictionary of attributes here :)

[awarzynski]

flang/include/flang/Optimizer/Dialect/FIROps.td

812 ↗

(On Diff #385108)

You need this to be able to parse the "pretty" form. But do you need one?

I don't think I do - thanks for pointing this out! :)

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

331 ↗

(On Diff #385108)

Ah, thanks! Yes, I will use that instead.

[awarzynski]

Remove the pretty printer and adjust the test accordingly

[mehdi_amini]

flang/lib/Optimizer/CodeGen/CodeGen.cpp

849

Nit: I think you can spell this rewriter.eraseOp for the same effect, but it expressed the intent a bit more clearly.

852

I was wondering if we need a pattern of its own for this op or if this should be handled by the parent operation?

The test shows it in isolation, but that does not exist, does it?

[mehdi_amini]

flang/lib/Optimizer/CodeGen/CodeGen.cpp

852

I even wonder if the verifier for fir.end shouldn't check on the actual parent op and limit it to be nested in some specific ones.

[awarzynski]

@mehdi_amini , these are much appreciated pointers, thank you!

flang/lib/Optimizer/CodeGen/CodeGen.cpp

852

I think that would make more sense. But, IIUC, in the current design there's only one such parent and it's only going to be needed for F2003. We are still working towards F95, so this is not required yet.

Perhaps we should just postpone this one until it's really needed?

[clementval]

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

332 ↗

(On Diff #385429)

It would probably make more sense to have a real use case where the op is actually used as a terminator.

[clementval]

flang/lib/Optimizer/CodeGen/CodeGen.cpp

849

+1

[awarzynski]

fir.end is only used in fir.dispatch_table, and fir.dispatch_table is currently a notifyMatchFailure, i.e. it's not implemented. I think that that's what we should do with fir.end as well. Let me update this accordingly. @schweitz , any thoughts?

[awarzynski]

Leave fir.end as unimplemented

[schweitz]

In D113295#3134860, @awarzynski wrote:

fir.end is only used in fir.dispatch_table, and fir.dispatch_table is currently a notifyMatchFailure, i.e. it's not implemented. I think that that's what we should do with fir.end as well. Let me update this accordingly. @schweitz , any thoughts?

Seems reasonable. fir.end used to be widely applicable, but it's steadily been eroded.

In fact, given https://llvm.discourse.group/t/rfc-making-terminator-optional-for-single-block-graph-regions/2997 it seems like fir.end will be end of life soon.

[clementval]

Can you update the commit message since it has change from the first version of your patch.

flang/lib/Optimizer/CodeGen/CodeGen.cpp

struct GenTypeDescOpConversion : public FIROpConversion<fir::GenTypeDescOp> {
   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
+  matchAndRewrite(fir::FirEndOp firEnd, OpAdaptor,
+                  mlir::ConversionPatternRewriter &rewriter) const override {
+    return rewriter.notifyMatchFailure(
+        firEnd, "fir.end codegen is not implemented yet");
+  }
+};
+
 /// Lower `fir.has_value` operation to `llvm.return` operation.
 struct HasValueOpConversion : public FIROpConversion<fir::HasValueOp> {
   using FIROpConversion::FIROpConversion;
 
   mlir::LogicalResult
   matchAndRewrite(fir::HasValueOp op, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
     rewriter.replaceOpWithNewOp<LLVM::ReturnOp>(op, adaptor.getOperands());
pattern.insert<
         AbsentOpConversion, AddcOpConversion, AddrOfOpConversion,
         AllocaOpConversion, BoxAddrOpConversion, BoxCharLenOpConversion,
         BoxDimsOpConversion, BoxEleSizeOpConversion, BoxIsAllocOpConversion,
         BoxIsArrayOpConversion, BoxIsPtrOpConversion, BoxProcHostOpConversion,
         BoxRankOpConversion, BoxTypeDescOpConversion, CallOpConversion,
         CmpcOpConversion, ConstcOpConversion, ConvertOpConversion,
         DispatchOpConversion, DispatchTableOpConversion, DTEntryOpConversion,
         DivcOpConversion, EmboxOpConversion, EmboxCharOpConversion,
-        EmboxProcOpConversion, ExtractValueOpConversion, HasValueOpConversion,
-        GenTypeDescOpConversion, GlobalLenOpConversion, GlobalOpConversion,
-        InsertOnRangeOpConversion, InsertValueOpConversion,
+        EmboxProcOpConversion, ExtractValueOpConversion, FirEndOpConversion,
+        HasValueOpConversion, GenTypeDescOpConversion, GlobalLenOpConversion,
+        GlobalOpConversion, InsertOnRangeOpConversion, InsertValueOpConversion,
         IsPresentOpConversion, LoadOpConversion, NegcOpConversion,
         MulcOpConversion, SelectCaseOpConversion, SelectOpConversion,
         SelectRankOpConversion, SelectTypeOpConversion, ShapeOpConversion,
         ShapeShiftOpConversion, ShiftOpConversion, SliceOpConversion,
         StoreOpConversion, StringLitOpConversion, SubcOpConversion,
         UnboxCharOpConversion, UnboxProcOpConversion, UndefOpConversion,
         UnreachableOpConversion, ZeroOpConversion>(typeConverter);
     mlir::populateStdToLLVMConversionPatterns(typeConverter, pattern);

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

func @emboxproc_test() {
   %host_vars = fir.alloca tuple<i32,f64>
 // expected-error@+1{{failed to legalize operation 'fir.emboxproc'}}
   %bproc = fir.emboxproc @method_impl, %host_vars : ((i32) -> (), !fir.ref<tuple<i32,f64>>) -> !fir.boxproc<(i32) -> ()>
   return
 }
 
 // Test that `fir.unboxproc` and `fir.boxproc_host` also fails to be legalized.
 // At the moment these cannot be tested since the `fir.boxproc` type does not have a conversion.
+
+// -----
+
+// Test `fir.end` conversion failure. Not implemented yet.
+
+// expected-error@+1{{failed to legalize operation 'fir.end'}}
+"fir.end"() : () -> ()