[SYCL][Fusion] Enable -Werror for fusion library (intel#12830)

jopperm · web-flow · commit 37cb495511c0 · 2024-03-13T15:50:56.000+01:00
Treat warnings as errors, and fix the last remaining warnings in the
fusion library.

---------

Signed-off-by: Julian Oppermann &lt;julian.oppermann@codeplay.com&gt;
diff --git a/sycl-fusion/CMakeLists.txt b/sycl-fusion/CMakeLists.txt
@@ -9,6 +9,14 @@ set(SYCL_JIT_BASE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
 # directories, similar to how clang/CMakeLists.txt does it.
 set(LLVM_SPIRV_INCLUDE_DIRS "${LLVM_MAIN_SRC_DIR}/../llvm-spirv/include")
 
+# Set library-wide warning options.
+set(SYCL_FUSION_WARNING_FLAGS -Wall -Wextra)
+
+option(SYCL_FUSION_ENABLE_WERROR "Treat all warnings as errors in SYCL kernel fusion library" ON)
+if(SYCL_FUSION_ENABLE_WERROR)
+  list(APPEND SYCL_FUSION_WARNING_FLAGS -Werror)
+endif(SYCL_FUSION_ENABLE_WERROR)
+
 if(WIN32)
   message(WARNING "Kernel fusion not yet supported on Windows")
 else(WIN32)
diff --git a/sycl-fusion/common/CMakeLists.txt b/sycl-fusion/common/CMakeLists.txt
@@ -5,6 +5,15 @@ add_llvm_library(sycl-fusion-common
    Support
 )
 
+target_compile_options(sycl-fusion-common PRIVATE ${SYCL_FUSION_WARNING_FLAGS})
+
+# Mark LLVM headers as system headers to ignore warnigns in them. This
+# classification remains intact even if the same path is added as a normal
+# include path in GCC and Clang.
+target_include_directories(sycl-fusion-common
+  SYSTEM PRIVATE
+  ${LLVM_MAIN_INCLUDE_DIR}
+)
 target_include_directories(sycl-fusion-common
   PUBLIC
   ${CMAKE_CURRENT_SOURCE_DIR}/include
diff --git a/sycl-fusion/jit-compiler/CMakeLists.txt b/sycl-fusion/jit-compiler/CMakeLists.txt
@@ -29,14 +29,23 @@ add_llvm_library(sycl-fusion
    ${LLVM_TARGETS_TO_BUILD}
 )
 
+target_compile_options(sycl-fusion PRIVATE ${SYCL_FUSION_WARNING_FLAGS})
+
+# Mark LLVM and SPIR-V headers as system headers to ignore warnigns in them.
+# This classification remains intact even if the same paths are added as normal
+# include paths in GCC and Clang.
+target_include_directories(sycl-fusion
+  SYSTEM PRIVATE
+  ${LLVM_MAIN_INCLUDE_DIR}
+  ${LLVM_SPIRV_INCLUDE_DIRS}
+)
 target_include_directories(sycl-fusion
   PUBLIC
   $<INSTALL_INTERFACE:include>
   $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
   $<BUILD_INTERFACE:${SYCL_JIT_BASE_DIR}/common/include>
   PRIVATE
   ${CMAKE_CURRENT_SOURCE_DIR}/lib
-  ${LLVM_SPIRV_INCLUDE_DIRS}
 )
 
 find_package(Threads REQUIRED)
diff --git a/sycl-fusion/jit-compiler/lib/fusion/FusionHelper.cpp b/sycl-fusion/jit-compiler/lib/fusion/FusionHelper.cpp
@@ -169,14 +169,13 @@ Expected<std::unique_ptr<Module>> helper::FusionHelper::addFusedKernel(
 
           const auto S = [&]() -> StringRef {
             switch (Info.Intern) {
-            default:
-            case jit_compiler::Internalization::None:
-              llvm_unreachable(
-                  "Only a valid internalization kind should be used");
             case jit_compiler::Internalization::Local:
               return LocalInternalizationStr;
             case jit_compiler::Internalization::Private:
               return PrivateInternalizationStr;
+            default:
+              llvm_unreachable(
+                  "Only a valid internalization kind should be used");
             }
           }();
           EmplaceBackIntern(Info, S);
diff --git a/sycl-fusion/passes/CMakeLists.txt b/sycl-fusion/passes/CMakeLists.txt
@@ -14,6 +14,15 @@ add_llvm_library(SYCLKernelFusion MODULE
   intrinsics_gen
 )
 
+target_compile_options(SYCLKernelFusion PRIVATE ${SYCL_FUSION_WARNING_FLAGS})
+
+# Mark LLVM headers as system headers to ignore warnigns in them. This
+# classification remains intact even if the same path is added as a normal
+# include path in GCC and Clang.
+target_include_directories(SYCLKernelFusion
+  SYSTEM PRIVATE
+  ${LLVM_MAIN_INCLUDE_DIR}
+)
 target_include_directories(SYCLKernelFusion
   PUBLIC
   ${CMAKE_CURRENT_SOURCE_DIR}
@@ -57,6 +66,15 @@ add_llvm_library(SYCLKernelFusionPasses
   TargetParser
 )
 
+target_compile_options(SYCLKernelFusionPasses PRIVATE ${SYCL_FUSION_WARNING_FLAGS})
+
+# Mark LLVM headers as system headers to ignore warnigns in them. This
+# classification remains intact even if the same path is added as a normal
+# include path in GCC and Clang.
+target_include_directories(SYCLKernelFusionPasses
+  SYSTEM PRIVATE
+  ${LLVM_MAIN_INCLUDE_DIR}
+)
 target_include_directories(SYCLKernelFusionPasses
   PUBLIC
   $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
diff --git a/sycl-fusion/passes/internalization/Internalization.cpp b/sycl-fusion/passes/internalization/Internalization.cpp
@@ -54,7 +54,7 @@ struct SYCLInternalizerImpl {
   TargetFusionInfo TargetInfo;
 
   /// Implements internalization the pass run.
-  PreservedAnalyses operator()(Module &M, ModuleAnalysisManager &AM) const;
+  PreservedAnalyses operator()(Module &M) const;
 
   ///
   /// Update a value to be promoted in a function.
@@ -71,8 +71,8 @@ struct SYCLInternalizerImpl {
   void promoteValue(Value *Val, const PromotionInfo &PromInfo,
                     bool InAggregate) const;
 
-  void promoteGEPI(GetElementPtrInst *GEPI, const Value *Val,
-                   const PromotionInfo &PromInfo, bool InAggregate) const;
+  void promoteGEPI(GetElementPtrInst *GEPI, const PromotionInfo &PromInfo,
+                   bool InAggregate) const;
 
   void promoteCall(CallBase *C, const Value *Val,
                    const PromotionInfo &PromInfo) const;
@@ -103,8 +103,8 @@ struct SYCLInternalizerImpl {
   ///
   /// Check that the operand of a GEP can be promoted to its users, and
   /// propagate whether it represents a pointer into an aggregate object.
-  Error canPromoteGEP(GetElementPtrInst *GEPI, const Value *Val,
-                      const PromotionInfo &PromInfo, bool InAggregate) const;
+  Error canPromoteGEP(GetElementPtrInst *GEPI, const PromotionInfo &PromInfo,
+                      bool InAggregate) const;
 
   ///
   /// Check if operand to a function call can be promoted.
@@ -356,7 +356,6 @@ static int getGEPKind(GetElementPtrInst *GEPI, const PromotionInfo &PromInfo) {
 }
 
 Error SYCLInternalizerImpl::canPromoteGEP(GetElementPtrInst *GEPI,
-                                          const Value *Val,
                                           const PromotionInfo &PromInfo,
                                           bool InAggregate) const {
   if (cast<PointerType>(GEPI->getType())->getAddressSpace() == AS) {
@@ -405,7 +404,7 @@ Error SYCLInternalizerImpl::canPromoteValue(Value *Val,
       }
       break;
     case Instruction::GetElementPtr:
-      if (auto Err = canPromoteGEP(cast<GetElementPtrInst>(I), Val, PromInfo,
+      if (auto Err = canPromoteGEP(cast<GetElementPtrInst>(I), PromInfo,
                                    InAggregate)) {
         return Err;
       }
@@ -488,7 +487,6 @@ void SYCLInternalizerImpl::promoteCall(CallBase *C, const Value *Val,
 }
 
 void SYCLInternalizerImpl::promoteGEPI(GetElementPtrInst *GEPI,
-                                       const Value *Val,
                                        const PromotionInfo &PromInfo,
                                        bool InAggregate) const {
   // Not PointerType is unreachable. Other case is caught in caller.
@@ -523,7 +521,7 @@ void SYCLInternalizerImpl::promoteValue(Value *Val,
       promoteCall(cast<CallBase>(I), Val, PromInfo);
       break;
     case Instruction::GetElementPtr:
-      promoteGEPI(cast<GetElementPtrInst>(I), Val, PromInfo, InAggregate);
+      promoteGEPI(cast<GetElementPtrInst>(I), PromInfo, InAggregate);
       break;
     case Instruction::Load:
     case Instruction::Store:
@@ -637,8 +635,7 @@ Function *SYCLInternalizerImpl::promoteFunctionArgs(
   return NewF;
 }
 
-PreservedAnalyses
-SYCLInternalizerImpl::operator()(Module &M, ModuleAnalysisManager &AM) const {
+PreservedAnalyses SYCLInternalizerImpl::operator()(Module &M) const {
   bool Changed{false};
   SmallVector<Function *> ToUpdate;
   for (auto &F : M) {
@@ -729,10 +726,10 @@ PreservedAnalyses llvm::SYCLInternalizer::run(Module &M,
   TargetFusionInfo TFI{&M};
   // Private promotion
   const PreservedAnalyses Tmp = SYCLInternalizerImpl{
-      TFI.getPrivateAddressSpace(), PrivatePromotion, true, TFI}(M, AM);
+      TFI.getPrivateAddressSpace(), PrivatePromotion, true, TFI}(M);
   // Local promotion
-  PreservedAnalyses Res = SYCLInternalizerImpl{
-      TFI.getLocalAddressSpace(), LocalPromotion, false, TFI}(M, AM);
+  PreservedAnalyses Res = SYCLInternalizerImpl{TFI.getLocalAddressSpace(),
+                                               LocalPromotion, false, TFI}(M);
 
   Res.intersect(Tmp);
 
diff --git a/sycl-fusion/passes/kernel-fusion/SYCLKernelFusion.cpp b/sycl-fusion/passes/kernel-fusion/SYCLKernelFusion.cpp
@@ -198,8 +198,7 @@ static bool needsGuard(const jit_compiler::NDRange &SrcNDRange,
 static FusionInsertPoints addGuard(IRBuilderBase &Builder,
                                    const TargetFusionInfo &TargetInfo,
                                    const jit_compiler::NDRange &SrcNDRange,
-                                   const jit_compiler::NDRange &FusedNDRange,
-                                   bool IsLast) {
+                                   const jit_compiler::NDRange &FusedNDRange) {
   // Guard:
 
   // entry:
@@ -240,8 +239,7 @@ static Expected<CallInst *> createFusionCall(
     const jit_compiler::NDRange &FusedNDRange, bool IsLast,
     jit_compiler::BarrierFlags BarriersFlags, jit_compiler::Remapper &Remapper,
     bool ShouldRemap, TargetFusionInfo &TargetInfo) {
-  const auto IPs =
-      addGuard(Builder, TargetInfo, SrcNDRange, FusedNDRange, IsLast);
+  const auto IPs = addGuard(Builder, TargetInfo, SrcNDRange, FusedNDRange);
 
   if (ShouldRemap) {
     auto FOrErr = Remapper.remapBuiltins(F, SrcNDRange, FusedNDRange);
diff --git a/sycl-fusion/passes/syclcp/SYCLCP.cpp b/sycl-fusion/passes/syclcp/SYCLCP.cpp
@@ -63,7 +63,7 @@ static Expected<SmallVector<ConstantInfo>> getCPFromMD(Function *F) {
 ///
 /// Returns a constant of the given scalar type and value.
 static Expected<Constant *> getConstantValue(const unsigned char **ValPtr,
-                                             Type *Ty, bool ByVal) {
+                                             Type *Ty) {
   if (Ty->isIntegerTy()) {
     unsigned NumBytes = Ty->getIntegerBitWidth() / 8;
     uint64_t IntValue = 0;
@@ -99,7 +99,7 @@ static Error initializeAggregateConstant(const unsigned char **ValPtr,
                                          ArrayRef<Value *> Indices) {
   if (CurrentTy->isIntegerTy() || CurrentTy->isFloatTy() ||
       CurrentTy->isDoubleTy()) {
-    Expected<Value *> CVal = getConstantValue(ValPtr, CurrentTy, false);
+    Expected<Value *> CVal = getConstantValue(ValPtr, CurrentTy);
     if (auto E = CVal.takeError()) {
       return E;
     }
@@ -185,8 +185,7 @@ static bool propagateConstants(Function *F, ArrayRef<ConstantInfo> Constants) {
       }
       CVal = AggVal.get();
     } else {
-      Expected<Constant *> ScalarVal =
-          getConstantValue(&ValPtr, ArgTy, Arg->hasByValAttr());
+      Expected<Constant *> ScalarVal = getConstantValue(&ValPtr, ArgTy);
       if (auto E = ScalarVal.takeError()) {
         handleAllErrors(std::move(E), [](const StringError &SE) {
           FUSION_DEBUG(llvm::dbgs() << SE.message() << "\n");
diff --git a/sycl-fusion/passes/target/TargetFusionInfo.cpp b/sycl-fusion/passes/target/TargetFusionInfo.cpp
@@ -408,8 +408,8 @@ class SPIRVTargetFusionInfo : public TargetFusionInfoImpl {
   }
 
   Function *createRemapperFunction(
-      const Remapper &R, BuiltinKind K, StringRef OrigName, Module *M,
-      const jit_compiler::NDRange &SrcNDRange,
+      const Remapper &R, BuiltinKind K, [[maybe_unused]] StringRef OrigName,
+      Module *M, const jit_compiler::NDRange &SrcNDRange,
       const jit_compiler::NDRange &FusedNDRange) const override {
     const auto Name = Remapper::getFunctionName(K, SrcNDRange, FusedNDRange);
     assert(!M->getFunction(Name) && "Function name should be unique");
@@ -551,7 +551,7 @@ class NVPTXAMDGCNTargetFusionInfoBase : public TargetFusionInfoImpl {
                                                   uint32_t Idx) const = 0;
 
   Value *getGlobalIDWithoutOffset(IRBuilderBase &Builder,
-                                  const NDRange &FusedNDRange,
+                                  [[maybe_unused]] const NDRange &FusedNDRange,
                                   uint32_t Idx) const override {
     // Construct (or reuse) a helper function to query the global ID.
     std::string GetGlobalIDName =
@@ -687,8 +687,9 @@ class NVPTXAMDGCNTargetFusionInfoBase : public TargetFusionInfoImpl {
     switch (K) {
     case BuiltinKind::NumWorkGroupsRemapper:
     case BuiltinKind::GroupIDRemapper:
-      return WrapValInFunc(
-          [&](uint32_t Idx) { return Builder.getInt32(R.getDefaultValue(K)); });
+      return WrapValInFunc([&]([[maybe_unused]] uint32_t Idx) {
+        return Builder.getInt32(R.getDefaultValue(K));
+      });
     case BuiltinKind::LocalSizeRemapper:
     case BuiltinKind::GlobalSizeRemapper: /* only AMDGCN */
       return WrapValInFunc([&](uint32_t Idx) {
