[Flang][OpenMP] Add --cg-rewrite pass to convert-to-llvm-openmp-and-fir.fir test

[agozillon]

Currently this is missing from the fir-opt command in this test, which means we can't use certain fir.operations inside of the test without causing verification issues. As an example fir::DeclareOp, which I believe the cg-rewrite pass normally converts to fir::cg::XDeclareOp prior to CodeGen'ng to LLVM-IR.

Perhaps, there's a reason for not including this option in the command list originally that I am not aware of! But it would be helpful to enable this pass to allow the full (or at least a wider) range of FIR operations to be used within the test cases.

[llvmbot]

@llvm/pr-subscribers-flang-openmp

Author: None (agozillon)

Changes

Currently this is missing from the fir-opt command in this test, which means we can't use certain fir.operations inside of the test without causing verification issues. As an example fir::DeclareOp, which I believe the cg-rewrite pass normally converts to fir::cg::XDeclareOp prior to CodeGen'ng to LLVM-IR.

Perhaps, there's a reason for not including this option in the command list originally that I am not aware of! But it would be helpful to enable this pass to allow the full (or at least a wider) range of FIR operations to be used within the test cases.

---

Full diff: https://github.com/llvm/llvm-project/pull/96530.diff

1 Files Affected:

• (modified) flang/test/Fir/convert-to-llvm-openmp-and-fir.fir (+1-1)

diff --git a/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir b/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
index 396fbaeacf39f..f8873ce57e913 100644
--- a/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
+++ b/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
@@ -1,4 +1,4 @@
-// RUN: fir-opt --split-input-file --cfg-conversion --fir-to-llvm-ir="target=aarch64-unknown-linux-gnu" %s | FileCheck %s
+// RUN: fir-opt --split-input-file --cg-rewrite --cfg-conversion --fir-to-llvm-ir="target=aarch64-unknown-linux-gnu" %s | FileCheck %s
 
 func.func @_QPsb1(%arg0: !fir.ref<i32> {fir.bindc_name = "n"}, %arg1: !fir.ref<!fir.array<?xi32>> {fir.bindc_name = "arr"}) {
   %c1_i64 = arith.constant 1 : i64

[abidh]

The fir::cg::XDeclareOp is quite a recent addition so it was not there when the test was originally written.

[agozillon]

The fir::cg::XDeclareOp is quite a recent addition so it was not there when the test was originally written.

Thank you very much @abidh ! Would adding the cg-rewrite pass to the fir-opt argument list be the correct fix in this case then?

The verification issue was there prior to the addition of fir::cg::XDeclareOp, however, It seems from looking at the previous code (and quickly testing it via regressing that section of code) for the DeclareOp rewrite inside of PreCGRewrtie.cpp it would be correct still, as it seems to replace all DeclareOp's with the corresponding memory reference (if I am understanding the prior rewrite code correctly at least) which allows the FIR -> LLVM dialect conversion to pass without verification issues.

[abidh]

The fir::cg::XDeclareOp is quite a recent addition so it was not there when the test was originally written.

Thank you very much @abidh ! Would adding the cg-rewrite pass to the fir-opt argument list be the correct fix in this case then?

The verification issue was there prior to the addition of fir::cg::XDeclareOp, however, It seems from looking at the previous code (and quickly testing it via regressing that section of code) for the DeclareOp rewrite inside of PreCGRewrtie.cpp it would be correct still, as it seems to replace all DeclareOp's with the corresponding memory reference (if I am understanding the prior rewrite code correctly at least) which allows the FIR -> LLVM dialect conversion to pass without verification issues.

I don't think it will be problem adding --cg-rewrite here as it is already running --cfg-conversion. Although I wonder if we are testing the conversion to llvm-ir, why not take the IR right before that pass and check that specific conversion. But I don't have the full context so feel free to ignore my comments.

[agozillon]

The fir::cg::XDeclareOp is quite a recent addition so it was not there when the test was originally written.

Thank you very much @abidh ! Would adding the cg-rewrite pass to the fir-opt argument list be the correct fix in this case then?
The verification issue was there prior to the addition of fir::cg::XDeclareOp, however, It seems from looking at the previous code (and quickly testing it via regressing that section of code) for the DeclareOp rewrite inside of PreCGRewrtie.cpp it would be correct still, as it seems to replace all DeclareOp's with the corresponding memory reference (if I am understanding the prior rewrite code correctly at least) which allows the FIR -> LLVM dialect conversion to pass without verification issues.

I don't think it will be problem adding --cg-rewrite here as it is already running --cfg-conversion. Although I wonder if we are testing the conversion to llvm-ir, why not take the IR right before that pass and check that specific conversion. But I don't have the full context so feel free to ignore my comments.

We could do that if we wish, I don't mind what direction we take, all it would require is an adjustment of the tests I added in another PR, but it does mean any IR that you generate via emit-fir can't be directly utilised in the test and you have to go out of your way to appropriately retrieve the IR directly before the pass. It's also came up a few times in our internal chat why certain FIR operations trigger verifier errors in certain cases, so generally nice to find out why in this case!

If @kiranchandramohan or @tblah or anyone else has any further input to push this PR in one direction or the other (e.g. closure or landing) it'd be appreciated.

[tblah]

I think it would be best to run only the conversion in this test so that if the test ever fails it is much easier to see what the input and output of each pass was.

This could be achieved by running those intermediate passes on the IR in the test manually, then updating the test file with the results.

I think there is some argument for using IR that has been through the full pass pipeline of flang-new (capture the output of flang-new -fc1 -emit-obj -fopenmp file.f90 -o /dev/null -mmlir --mlir-print-ir-after-all then take the version of the IR from immediately before the conversion to LLVM). This would be the most realistic test. But at the same time I worry this could lead to overly verbose IR in the test that is harder to read and maintain.

[agozillon]

Sounds good, so we're leaning towards closing this PR then and not altering the test! I'll leave this open for a day or so incase of any other responses otherwise I'll close it.

[agozillon]

Closing this PR as we've decided it's an unnecessary change and it would likely be better to create another test file encompassing the full end-to-end transform/optimisation flow if we wanted to do tests like this!

Thank you very much everyone for your input and time :-)