Skip to content

Conversation

@mgudim
Copy link
Contributor

@mgudim mgudim commented Nov 18, 2025

Currently the test cfi-multiple-location.mir is marked as XFAIL. This causes failures on some build bots because the test unexpectedly passes.

Mark this test as UNSUPPORTED for now. Later I plan to merge an MR which fixes an issue in CFIInstrInserter and this test will be enabled.

@llvmbot
Copy link
Member

llvmbot commented Nov 18, 2025

@llvm/pr-subscribers-backend-risc-v

Author: Mikhail Gudim (mgudim)

Changes

Currently the test cfi-multiple-location.mir is marked as XFAIL. This causes failures on some build bots because the test unexpectedly passes.

Mark this test as UNSUPPORTED for now. Later I plan to merge an MR which fixes an issue in CFIInstrInserter and this test will be enabled.


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

1 Files Affected:

  • (modified) llvm/test/CodeGen/RISCV/cfi-multiple-locations.mir (+1-1)
diff --git a/llvm/test/CodeGen/RISCV/cfi-multiple-locations.mir b/llvm/test/CodeGen/RISCV/cfi-multiple-locations.mir
index 7844589e3f93c..6a447f5fff174 100644
--- a/llvm/test/CodeGen/RISCV/cfi-multiple-locations.mir
+++ b/llvm/test/CodeGen/RISCV/cfi-multiple-locations.mir
@@ -1,7 +1,7 @@
 # RUN: llc %s -mtriple=riscv64 \
 # RUN: -run-pass=cfi-instr-inserter \
 # RUN: -riscv-enable-cfi-instr-inserter=true
-# XFAIL: *
+# UNSUPPORTED: *
 
 # Technically, it is possible that a callee-saved register is saved in multiple different locations.
 # CFIInstrInserter should handle this, but currently it does not.

Currently the test cfi-multiple-location.mir is marked as XFAIL. This
causes failures on some build bots because the test unexpectedly passes.

Mark this test as UNSUPPORTED for now. Later I plan to merge an MR which
fixes an issue in CFIInstrInserter and this test will be enabled.
@jrtc27
Copy link
Collaborator

jrtc27 commented Nov 18, 2025

Why is this non-deterministic?

@mgudim
Copy link
Contributor Author

mgudim commented Nov 18, 2025

@lenary @ppenzin alternatively, we can just merge #168531

@mgudim
Copy link
Contributor Author

mgudim commented Nov 18, 2025

Why is this non-deterministic?

Actually I don't know why it successfully XFAIL on some buildbots, but not on others. When the test runs llc will hit an assertion in the code. Maybe there is a difference in what happens next?

@mgudim
Copy link
Contributor Author

mgudim commented Nov 18, 2025

I ran the test locally and it worked as I expected. I suspect it will be a lot of work to see why exactly it fails on some buildbots. Since this is only XFAIL temporarily, I hope it would be OK just to make this UNSUPPORTED.

@mgudim mgudim requested a review from jrtc27 November 18, 2025 13:19
@github-actions
Copy link

🐧 Linux x64 Test Results

  • 186287 tests passed
  • 4854 tests skipped

Copy link
Member

@lenary lenary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving so this can fix the bots.

I presume the issue is some bots have asserts enabled and some don't

@mgudim mgudim merged commit 04a1fd5 into llvm:main Nov 18, 2025
10 checks passed
@mgudim
Copy link
Contributor Author

mgudim commented Nov 18, 2025

Approving so this can fix the bots.

I presume the issue is some bots have asserts enabled and some don't

@lenary oh, right. I guess the proper fix would have been to add "requires asserts".

@dyung
Copy link
Collaborator

dyung commented Nov 18, 2025

Approving so this can fix the bots.
I presume the issue is some bots have asserts enabled and some don't

@lenary oh, right. I guess the proper fix would have been to add "requires asserts".

This is probably the correct fix. I also saw this test failure on my bot that specifically builds without asserts: https://lab.llvm.org/buildbot/#/builders/202/builds/4233

@topperc
Copy link
Collaborator

topperc commented Nov 18, 2025

Approving so this can fix the bots.
I presume the issue is some bots have asserts enabled and some don't

@lenary oh, right. I guess the proper fix would have been to add "requires asserts".

This is probably the correct fix. I also saw this test failure on my bot that specifically builds without asserts: https://lab.llvm.org/buildbot/#/builders/202/builds/4233

The correct fix is to change the llvm_unreachable to a reportFatalError. If there is an input that can reach an llvm_unreachable, then it is not "unreachable"

topperc added a commit to topperc/llvm-project that referenced this pull request Nov 19, 2025
This prevents it from being optimized out in non-assert builds.

Update X86 test to remove REQUIRES: asserts and check for LLVM ERROR.
Add FileCheck to RISC-V test and remove UNSUPPORTED.

This is the better fix for llvm#168772 and llvm#168525.
topperc added a commit that referenced this pull request Nov 20, 2025
…rror. (#168777)

This prevents it from being optimized out in non-asserts builds.

Update X86 test to remove REQUIRES: asserts and check for LLVM ERROR.
Add FileCheck to RISC-V test and remove UNSUPPORTED.

This is the more complete fix for #168772 and #168525.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants