Skip to content

Conversation

@marcauberer
Copy link
Member

As discussed in this forum question, we hereby contribute the first SAP e2e test, that tests our common usage of indirectbr/blockaddr to achieve systematic outlining of functions.

@marcauberer marcauberer changed the title Add regression bitcode test for SAP usecase [RegressionTest] Add regression bitcode test for SAP usecase Nov 7, 2025
@MatzeB
Copy link
Contributor

MatzeB commented Nov 7, 2025

  • Haven't looked at the test in detail, but if this is just about indirectbr/block-addr can also be created with the "computed goto" construct in C/C++, this somehow does not work here?
  • Not sure this needs to spell "sap" that prominently in directory/test name; seems like just a test about indirectbr to me.
  • "due to a not yet fixed issue," you should not check in failing tests. If this has been fixed in the meantime, update the README
  • I think it's fine to just check in the .bc file; it's easy enough to disassemble and the .ll file is just at risk to get out of sync and bitrot in syntax (.bc is meant to still be readable with future LLVM versions, .ll syntax typically changes (slightly) between LLVM versions).

@MatzeB
Copy link
Contributor

MatzeB commented Nov 7, 2025

I don't know when/where the issue shows exactly, but maybe you can run -O1 or at least addr2reg passes on the input to shrink it to something shorter/more to the point.

@mshockwave
Copy link
Member

I don't know when/where the issue shows exactly, but maybe you can run -O1 or at least addr2reg passes on the input to shrink it to something shorter/more to the point.

You can go even further to use llvm-reduce to shrink the test size: https://llvm.org/docs/CommandGuide/llvm-reduce.html

@marcauberer
Copy link
Member Author

  • Haven't looked at the test in detail, but if this is just about indirectbr/block-addr can also be created with the "computed goto" construct in C/C++, this somehow does not work here?

This is not what we want here. The purpose of this test (and following) is, that we want to test IR, that was produced by our compiler. Thus, I

  1. took the IR as is (changed a few function names to link against the mock of course)
  2. did not try to minimize it, as it should stay representable to the output of our compiler

We are aware that LLVM tests are generally minimally reduced. This is why I first inquired the community opinion in the forum post and labeled the tests as end-to-end test.

  • Not sure this needs to spell "sap" that prominently in directory/test name; seems like just a test about indirectbr to me.

This is also because of the above mentioned purpose of this test. This way it is visible that we are the test owners and avoids that someone updates the test without us knowing.

  • "due to a not yet fixed issue," you should not check in failing tests. If this has been fixed in the meantime, update the README

I removed that part from the README, since when the issue is fixed will be outdated anyways. Maybe this part was also sub optimally phrased, because the IR of this test does (intentionally) not trigger the bug. We will of course include a regression test when fixing the issue in the LLVM main repo.

  • I think it's fine to just check in the .bc file; it's easy enough to disassemble and the .ll file is just at risk to get out of sync and bitrot in syntax (.bc is meant to still be readable with future LLVM versions, .ll syntax typically changes (slightly) between LLVM versions).

This part is done.

Copy link

@hubert-reinterpretcast hubert-reinterpretcast left a comment

Choose a reason for hiding this comment

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

Please change the PR title/subject to be more descriptive: "SAP usecase" is unclear.

@marcauberer marcauberer changed the title [RegressionTest] Add regression bitcode test for SAP usecase [RegressionTest] Add bitcode E2E test by SAP for cost-based function outlining with indirectbr/blockaddr Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants