Fallthrough (#4843)

A common source of bugs. Although many of these were harmless, some were
not. By eliminating them and enabling this warning as an error, we won't
add more in the future.


Fixed a real SPIRV bug that required a test update to expect the correct behavior.
SPIRV testing was expecting incorrect results that came from a fallthrough error

Author: Greg Roth

cmake/modules/HandleLLVMOptions.cmake

@@ -379,7 +379,7 @@ if( MSVC )
 
 elseif( LLVM_COMPILER_IS_GCC_COMPATIBLE )
   if (LLVM_ENABLE_WARNINGS)
-    append("-Wall -W -Wno-unused-parameter -Wwrite-strings" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+    append("-Wall -W -Wno-unused-parameter -Wwrite-strings -Wimplicit-fallthrough" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
     append("-Wcast-qual" CMAKE_CXX_FLAGS)
 
     # Disable unknown pragma warnings because the output is just too long with them.

external/CMakeLists.txt

@@ -39,6 +39,11 @@ if (${ENABLE_SPIRV_CODEGEN})
 
   if (NOT TARGET SPIRV-Tools)
     if (IS_DIRECTORY ${DXC_SPIRV_TOOLS_DIR})
+      # Avoid implicit fallthrough warning from clang
+      # This add_compile_options() will only affect the current directory and its subdirectories.
+      if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
+        add_compile_options(-Wno-implicit-fallthrough)
+      endif(NOT WIN32)
       # We only need the library from SPIRV-Tools.
       set(SPIRV_SKIP_EXECUTABLES ON CACHE BOOL "Skip building SPIRV-Tools executables")
       if (NOT HLSL_ENABLE_DEBUG_ITERATORS)

include/dxc/DXIL/DxilSigPoint.inl

@@ -166,11 +166,13 @@ DXIL::SigPointKind SigPoint::GetKind(DXIL::ShaderKind shaderKind, DXIL::Signatur
   if (isSpecialInput) {
     switch (shaderKind) {
     case DXIL::ShaderKind::Hull:
-    if (sigKind == DXIL::SignatureKind::Input)
-      return isPatchConstantFunction ? DXIL::SigPointKind::PCIn : DXIL::SigPointKind::HSIn;
+      if (sigKind == DXIL::SignatureKind::Input)
+        return isPatchConstantFunction ? DXIL::SigPointKind::PCIn : DXIL::SigPointKind::HSIn;
+          break;
     case DXIL::ShaderKind::Geometry:
       if (sigKind == DXIL::SignatureKind::Input)
         return DXIL::SigPointKind::GSIn;
+      break;
     default:
       break;
     }

include/dxc/Support/WinAdapter.h

@@ -312,7 +312,6 @@
 
 #define _Printf_format_string_
 #define _Null_terminated_
-#define __fallthrough
 
 #define _Field_size_(size)
 #define _Field_size_full_(size)

include/llvm/Support/Compiler.h

@@ -220,6 +220,17 @@
 #define LLVM_ATTRIBUTE_RETURNS_NOALIAS
 #endif
 
+#if __cplusplus > 201402L
+#define LLVM_FALLTHROUGH [[fallthrough]]
+#elif defined(__clang__)
+#define LLVM_FALLTHROUGH [[clang::fallthrough]]
+#elif defined(_MSC_VER)
+#define LLVM_FALLTHROUGH __fallthrough
+#else
+#define LLVM_FALLTHROUGH [[gnu::fallthrough]]
+#endif
+
+
 /// LLVM_EXTENSION - Support compilers where we have a keyword to suppress
 /// pedantic diagnostics.
 #ifdef __GNUC__

lib/Analysis/BasicAliasAnalysis.cpp

@@ -216,7 +216,7 @@ static Value *GetLinearExpression(Value *V, APInt &Scale, APInt &Offset,
         if (!MaskedValueIsZero(BOp->getOperand(0), RHSC->getValue(), DL, 0, AC,
                                BOp, DT))
           break;
-        // FALL THROUGH.
+        LLVM_FALLTHROUGH; // HLSL Change
       case Instruction::Add:
         V = GetLinearExpression(BOp->getOperand(0), Scale, Offset, Extension,
                                 DL, Depth + 1, AC, DT);

lib/Analysis/InstructionSimplify.cpp

@@ -2721,7 +2721,7 @@ static Value *SimplifyICmpInst(unsigned Predicate, Value *LHS, Value *RHS,
                      Q.CxtI, Q.DT);
       if (!KnownNonNegative)
         break;
-      // fall-through
+      LLVM_FALLTHROUGH; // HLSL Change
     case ICmpInst::ICMP_EQ:
     case ICmpInst::ICMP_UGT:
     case ICmpInst::ICMP_UGE:
@@ -2732,7 +2732,7 @@ static Value *SimplifyICmpInst(unsigned Predicate, Value *LHS, Value *RHS,
                      Q.CxtI, Q.DT);
       if (!KnownNonNegative)
         break;
-      // fall-through
+      LLVM_FALLTHROUGH; // HLSL Change
     case ICmpInst::ICMP_NE:
     case ICmpInst::ICMP_ULT:
     case ICmpInst::ICMP_ULE:
@@ -2752,7 +2752,7 @@ static Value *SimplifyICmpInst(unsigned Predicate, Value *LHS, Value *RHS,
                      Q.CxtI, Q.DT);
       if (!KnownNonNegative)
         break;
-      // fall-through
+      LLVM_FALLTHROUGH; // HLSL Change
     case ICmpInst::ICMP_NE:
     case ICmpInst::ICMP_UGT:
     case ICmpInst::ICMP_UGE:
@@ -2763,7 +2763,7 @@ static Value *SimplifyICmpInst(unsigned Predicate, Value *LHS, Value *RHS,
                      Q.CxtI, Q.DT);
       if (!KnownNonNegative)
         break;
-      // fall-through
+      LLVM_FALLTHROUGH; // HLSL Change
     case ICmpInst::ICMP_EQ:
     case ICmpInst::ICMP_ULT:
     case ICmpInst::ICMP_ULE:
@@ -2823,7 +2823,7 @@ static Value *SimplifyICmpInst(unsigned Predicate, Value *LHS, Value *RHS,
     case Instruction::LShr:
       if (ICmpInst::isSigned(Pred))
         break;
-      // fall-through
+      LLVM_FALLTHROUGH; // HLSL Change
     case Instruction::SDiv:
     case Instruction::AShr:
       if (!LBO->isExact() || !RBO->isExact())

lib/Analysis/MemoryDependenceAnalysis.cpp

@@ -641,6 +641,7 @@ MemDepResult MemoryDependenceAnalysis::getPointerDependencyFrom(
       // load query, we can safely ignore it (scan past it).
       if (isLoad)
         continue;
+      LLVM_FALLTHROUGH; // HLSL Change
     default:
       // Otherwise, there is a potential dependence.  Return a clobber.
       return MemDepResult::getClobber(Inst);
@@ -1014,6 +1015,7 @@ SortNonLocalDepInfoCache(MemoryDependenceAnalysis::NonLocalDepInfo &Cache,
       std::upper_bound(Cache.begin(), Cache.end()-1, Val);
     Cache.insert(Entry, Val);
     // FALL THROUGH.
+    LLVM_FALLTHROUGH; // HLSL Change
   }
   case 1:
     // One new entry, Just insert the new value at the appropriate position.

lib/Analysis/ScalarEvolution.cpp

@@ -4380,7 +4380,7 @@ const SCEV *ScalarEvolution::createSCEV(Value *V) {
       case ICmpInst::ICMP_SLT:
       case ICmpInst::ICMP_SLE:
         std::swap(LHS, RHS);
-        // fall through
+        LLVM_FALLTHROUGH; // HLSL Change
       case ICmpInst::ICMP_SGT:
       case ICmpInst::ICMP_SGE:
         // a >s b ? a+x : b+x  ->  smax(a, b)+x
@@ -4404,7 +4404,7 @@ const SCEV *ScalarEvolution::createSCEV(Value *V) {
       case ICmpInst::ICMP_ULT:
       case ICmpInst::ICMP_ULE:
         std::swap(LHS, RHS);
-        // fall through
+        LLVM_FALLTHROUGH; // HLSL Change
       case ICmpInst::ICMP_UGT:
       case ICmpInst::ICMP_UGE:
         // a >u b ? a+x : b+x  ->  umax(a, b)+x
@@ -4458,7 +4458,7 @@ const SCEV *ScalarEvolution::createSCEV(Value *V) {
       default:
         break;
       }
-    }
+    } break;
 
   default: // We cannot analyze this expression.
     break;
@@ -6696,6 +6696,7 @@ ScalarEvolution::isKnownPredicateWithRanges(ICmpInst::Predicate Pred,
     llvm_unreachable("Unexpected ICmpInst::Predicate value!");
   case ICmpInst::ICMP_SGT:
     std::swap(LHS, RHS);
+    LLVM_FALLTHROUGH; // HLSL Change
   case ICmpInst::ICMP_SLT: {
     ConstantRange LHSRange = getSignedRange(LHS);
     ConstantRange RHSRange = getSignedRange(RHS);
@@ -6707,6 +6708,7 @@ ScalarEvolution::isKnownPredicateWithRanges(ICmpInst::Predicate Pred,
   }
   case ICmpInst::ICMP_SGE:
     std::swap(LHS, RHS);
+    LLVM_FALLTHROUGH; // HLSL Change
   case ICmpInst::ICMP_SLE: {
     ConstantRange LHSRange = getSignedRange(LHS);
     ConstantRange RHSRange = getSignedRange(RHS);
@@ -6718,6 +6720,7 @@ ScalarEvolution::isKnownPredicateWithRanges(ICmpInst::Predicate Pred,
   }
   case ICmpInst::ICMP_UGT:
     std::swap(LHS, RHS);
+    LLVM_FALLTHROUGH; // HLSL Change
   case ICmpInst::ICMP_ULT: {
     ConstantRange LHSRange = getUnsignedRange(LHS);
     ConstantRange RHSRange = getUnsignedRange(RHS);
@@ -6729,6 +6732,7 @@ ScalarEvolution::isKnownPredicateWithRanges(ICmpInst::Predicate Pred,
   }
   case ICmpInst::ICMP_UGE:
     std::swap(LHS, RHS);
+    LLVM_FALLTHROUGH; // HLSL Change
   case ICmpInst::ICMP_ULE: {
     ConstantRange LHSRange = getUnsignedRange(LHS);
     ConstantRange RHSRange = getUnsignedRange(RHS);
@@ -7052,6 +7056,7 @@ bool ScalarEvolution::isImpliedCond(ICmpInst::Predicate Pred,
           if (isImpliedCondOperands(Pred, LHS, RHS, V,
                                     getConstant(SharperMin)))
             return true;
+          LLVM_FALLTHROUGH; // HLSL Change
 
         case ICmpInst::ICMP_SGT:
         case ICmpInst::ICMP_UGT:
@@ -7066,7 +7071,7 @@ bool ScalarEvolution::isImpliedCond(ICmpInst::Predicate Pred,
 
           if (isImpliedCondOperands(Pred, LHS, RHS, V, getConstant(Min)))
             return true;
-
+          break;
         default:
           // No change
           break;
@@ -7163,7 +7168,7 @@ static bool IsKnownPredicateViaMinOrMax(ScalarEvolution &SE,
 
   case ICmpInst::ICMP_SGE:
     std::swap(LHS, RHS);
-    // fall through
+    LLVM_FALLTHROUGH; // HLSL Change
   case ICmpInst::ICMP_SLE:
     return
       // min(A, ...) <= A
@@ -7173,7 +7178,7 @@ static bool IsKnownPredicateViaMinOrMax(ScalarEvolution &SE,
 
   case ICmpInst::ICMP_UGE:
     std::swap(LHS, RHS);
-    // fall through
+    LLVM_FALLTHROUGH; // HLSL Change
   case ICmpInst::ICMP_ULE:
     return
       // min(A, ...) <= A
@@ -8423,6 +8428,7 @@ ScalarEvolution::computeBlockDisposition(const SCEV *S, const BasicBlock *BB) {
       return DoesNotDominateBlock;
   }
   // FALL THROUGH into SCEVNAryExpr handling.
+  LLVM_FALLTHROUGH; // HLSL Change
   case scAddExpr:
   case scMulExpr:
   case scUMaxExpr:

lib/Analysis/ValueTracking.cpp

@@ -2182,6 +2182,7 @@ bool llvm::ComputeMultiple(Value *V, unsigned Base, Value *&Multiple,
   case Instruction::SExt:
     if (!LookThroughSExt) return false;
     // otherwise fall through to ZExt
+    LLVM_FALLTHROUGH; // HLSL Change
   case Instruction::ZExt:
     return ComputeMultiple(I->getOperand(0), Base, Multiple,
                            LookThroughSExt, Depth+1);
@@ -2331,7 +2332,7 @@ bool llvm::CannotBeOrderedLessThanZero(const Value *V, unsigned Depth) {
     // x*x is always non-negative or a NaN.
     if (I->getOperand(0) == I->getOperand(1)) 
       return true;
-    // Fall through
+    LLVM_FALLTHROUGH; // HLSL Change
   case Instruction::FAdd:
   case Instruction::FDiv:
   case Instruction::FRem: