diffkemp icon indicating copy to clipboard operation
diffkemp copied to clipboard

Bump up the LLVM version to 19

Open DanielKriz opened this issue 3 months ago • 5 comments

This PR adds support for LLVM 19 to diffkemp.

The main differences in comparison to older versions are:

  1. The removal of comparison of StringRef with equals method. We should investigate when was the == operator added and use that instead of some macro and intermediate function.
  2. It seems that some includes regarding the Module and GlobalValue have changed. It was sufficient to add their includes explicitly.
  3. The getPredicate method was removed from ConstExpr and the method that was extracted into a standalone function have many changes (getConstExprAsInstruction).

Note: We do not implement the cmpAPFloats, we should either implement it, or document why not? Note: We should document the process of adding a new FunctionComparator to the code base, because I have again burned a lot of time on a super simple mistake.

Currently the unit tests fail, this requires a bit of investigation from my side.

DanielKriz avatar Sep 15 '25 19:09 DanielKriz

@DanielKriz It looks like lowerswitch LLVM optimization pass was renamed to lower-switch [1]. It will need update in https://github.com/diffkemp/diffkemp/blob/8912507d38d3a9591ee55f00a6d7524b204d0255/diffkemp/llvm_ir/optimiser.py#L17

[1] https://github.com/llvm/llvm-project/commit/e390c229a438ed1eb3396df8fbeeda89c49474e6

PLukas2018 avatar Sep 16 '25 07:09 PLukas2018

  1. The removal of comparison of StringRef with equals method. We should investigate when was the == operator added and use that instead of some macro and intermediate function.

It's been supported since at least LLVM 3.0 so let's use it directly.

Note: We do not implement the cmpAPFloats, we should either implement it, or document why not?

Hm, not sure, probably because kernel (which was our first target) doesn't do FP operations. But we should probably find some benchmark and add it.

Note: We should document the process of adding a new FunctionComparator to the code base, because I have again burned a lot of time on a super simple mistake.

That's a great idea.

viktormalik avatar Sep 16 '25 11:09 viktormalik

All right, I think that it is ready for the first true review (at the moment of writing this comment I have pushed some change to the documentation, so the CI might not have passed through yet; however, before that it did).

There are some things regarding commits that I need to discuss:

  1. I have found out, that I forgot to add LLVM 18 to the list of tested versions in CI, I think that it should be a stand-alone commit, becuause of its "semantics", but it is a fix comment.
  2. Thanks to the help of @PLukas2018 the tests are now passing, because the Diffkemp now has to support new approach to debug info. It is quite a big change and it has to be squashed to the main LLVM 19 commit, but I want to make this review in separation, before I carefully merge the commit messages.

@viktormalik could you take look on this?

DanielKriz avatar Sep 26 '25 07:09 DanielKriz

Before looking in detail, I think we should split this a bit more:

  • the first two commits can go to a separate PR, along with the last commit
  • the docs commit can also go to a separate PR

In addition,

  1. Thanks to the help of @PLukas2018 the tests are now passing, because the Diffkemp now has to support new approach to debug info. It is quite a big change and it has to be squashed to the main LLVM 19 commit, but I want to make this review in separation, before I carefully merge the commit messages.

This is not true, you can split the LLVM 19 update commit into multiple commits. First, you can do each required change separately (#if LLVM_VERSION_MAJOR >= 19 will just always be false) and as the last commit, you can add "support for LLVM 19" (i.e. new FunctionComparator, CMakeLists.txt changes, and CI jobs).

These splits will make the PR much easier to review.

Thanks!

viktormalik avatar Sep 26 '25 09:09 viktormalik

@viktormalik It should be ready for a review.

DanielKriz avatar Nov 13 '25 13:11 DanielKriz

The comment is rephased, and it should be ready for a merge @viktormalik .

DanielKriz avatar Nov 20 '25 07:11 DanielKriz

Great work!

viktormalik avatar Nov 21 '25 10:11 viktormalik