-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Flang][OpenMP] Add --cg-rewrite pass to convert-to-llvm-openmp-and-fir.fir test #96530
Conversation
…ir.fir test 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.
@llvm/pr-subscribers-flang-openmp Author: None (agozillon) ChangesCurrently 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:
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
|
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 |
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. |
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 |
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. |
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 :-) |
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.