Skip to content
This repository was archived by the owner on May 9, 2024. It is now read-only.

Update to LLVM 15 #633

Merged
merged 7 commits into from
Aug 18, 2023
Merged

Update to LLVM 15 #633

merged 7 commits into from
Aug 18, 2023

Conversation

alexbaden
Copy link
Contributor

LLVM 16 requires opaque pointers, which we currently can't support (mostly due to use of getPointerElementType() almost everywhere). In LLVM 14, opaque pointers don't seem to work for us (appears to be an issue in the optimization pipeline or elsewhere in the JIT when reading the module). This PR attempts an intermediate upgrade to LLVM 15 with opaque pointers disabled. If this works, we can then enable opaque pointers under LLVM 15, and then the upgrade to 16 should be smooth.

@alexbaden alexbaden force-pushed the alex/llvm15 branch 2 times, most recently from 6c8346f to cf5f3d5 Compare August 16, 2023 23:10
(consistent treatment of single line functions)
Windows requires a patch to the Findzstd cmake file
(llvm/llvm-project@e7fc754)
which is available in LLVM 16, not LLVM 15.
@alexbaden alexbaden marked this pull request as ready for review August 17, 2023 16:04
@alexbaden
Copy link
Contributor Author

Hopefully this passes tests now - either way it is ready for review. This upgrades our Linux conda builds to LLVM 15 (including clang-format) but continues to run LLVM 14 on Windows and in Docker containers. LLVM 15 is a stepping stone because it supports opaque pointers, but does not require them (LLVM 14 does not support them, LLVM 16 requires them). However, it has bugs in CMake which effect Windows (resolved in LLVM 16). So, the plan is to run LLVM 15 in Conda for a bit while we enable opaque pointers, then upgrade to LLVM 16 on all machines.

If we have to do a release we should be fine to release on LLVM 14 or LLVM 15 (except on Windows) - I did not update meta.yml (though I am not sure that is stilled used by conda-forge). Developers should update conda once this is merged to pickup clang-format improvements.

@kurapov-peter this seems to pass L0 tests but your eyes on the L0 changes would be appreciated!

Copy link
Contributor

@kurapov-peter kurapov-peter left a comment

Choose a reason for hiding this comment

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

I rebased this on top of the current main locally. Enabling tests seem to work fine with both llvm 14 and 15 even on PVC, so I think we could even update the docker.

Smth like this should do:

diff --git a/docker/Dockerfile b/docker/Dockerfile
index c1351319e..037cc3890 100644
--- a/docker/Dockerfile
+++ b/docker/Dockerfile
@@ -31,15 +31,15 @@ RUN apt-get update && apt-get install -y \
 # LLVM
 RUN wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | apt-key add - && \
     apt-add-repository \
-        'deb http://apt.llvm.org/focal/ llvm-toolchain-focal-14 main' && \
+        'deb http://apt.llvm.org/focal/ llvm-toolchain-focal-15 main' && \
     apt-get update && apt-get install -y \
-        llvm-14 \
-        llvm-14-dev \
-        clang-14 \
-        libclang-14-dev \
+        llvm-15 \
+        llvm-15-dev \
+        clang-15 \
+        libclang-15-dev \
         --
 
-ENV PATH=/usr/lib/llvm-14/bin${PATH:+:${PATH}}
+ENV PATH=/usr/lib/llvm-15/bin${PATH:+:${PATH}}
 
 # Dependencies
 RUN apt-get update && apt-get install -y \
diff --git a/docker/Dockerfile.l0 b/docker/Dockerfile.l0
index e626e86ea..09144a66e 100644
--- a/docker/Dockerfile.l0
+++ b/docker/Dockerfile.l0
@@ -12,7 +12,7 @@ RUN mkdir /intel-gpu-drivers && cd /intel-gpu-drivers && \
 
 RUN cd /intel-gpu-drivers && dpkg -i *.deb
 
-RUN git clone -b llvm_release_140 https://github.com/KhronosGroup/SPIRV-LLVM-Translator.git && \
+RUN git clone -b llvm_release_150 https://github.com/KhronosGroup/SPIRV-LLVM-Translator.git && \
     mkdir SPIRV-LLVM-Translator/build && cd SPIRV-LLVM-Translator/build && \
     cmake -Wno-dev .. && make llvm-spirv -j`nproc` && make install

@@ -15,6 +15,7 @@
#pragma once

#include <llvm/IR/Value.h>
#include <llvm/Transforms/IPO/PassManagerBuilder.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should live in the cpp I think.

@alexbaden alexbaden merged commit 8086560 into main Aug 18, 2023
@alexbaden alexbaden deleted the alex/llvm15 branch August 18, 2023 15:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants