Skip to content

Conversation

dpaoliello
Copy link
Contributor

The change #137258 introduced a build break on newer versions of MSVC:

llvm\include\llvm\Analysis\DXILResource.h(674) : warning C4715: 'llvm::DXILResourceBindingInfo::getBindingSpaces': not all control paths return a value

Fix is to add a default case that will ICE.

@dpaoliello dpaoliello requested a review from hekota May 9, 2025 18:53
@llvmbot llvmbot added backend:DirectX llvm:analysis Includes value tracking, cost tables and constant folding labels May 9, 2025
@llvmbot
Copy link
Member

llvmbot commented May 9, 2025

@llvm/pr-subscribers-llvm-analysis

Author: Daniel Paoliello (dpaoliello)

Changes

The change #137258 introduced a build break on newer versions of MSVC:

llvm\include\llvm\Analysis\DXILResource.h(674) : warning C4715: 'llvm::DXILResourceBindingInfo::getBindingSpaces': not all control paths return a value

Fix is to add a default case that will ICE.


Full diff: https://github.com/llvm/llvm-project/pull/139309.diff

1 Files Affected:

  • (modified) llvm/include/llvm/Analysis/DXILResource.h (+2)
diff --git a/llvm/include/llvm/Analysis/DXILResource.h b/llvm/include/llvm/Analysis/DXILResource.h
index cfeed5c4ef76f..090b6e77eac00 100644
--- a/llvm/include/llvm/Analysis/DXILResource.h
+++ b/llvm/include/llvm/Analysis/DXILResource.h
@@ -670,6 +670,8 @@ class DXILResourceBindingInfo {
       return CBufferSpaces;
     case dxil::ResourceClass::Sampler:
       return SamplerSpaces;
+    default:
+      llvm_unreachable("Invalid resource class");
     }
   }
 

@llvmbot
Copy link
Member

llvmbot commented May 9, 2025

@llvm/pr-subscribers-backend-directx

Author: Daniel Paoliello (dpaoliello)

Changes

The change #137258 introduced a build break on newer versions of MSVC:

llvm\include\llvm\Analysis\DXILResource.h(674) : warning C4715: 'llvm::DXILResourceBindingInfo::getBindingSpaces': not all control paths return a value

Fix is to add a default case that will ICE.


Full diff: https://github.com/llvm/llvm-project/pull/139309.diff

1 Files Affected:

  • (modified) llvm/include/llvm/Analysis/DXILResource.h (+2)
diff --git a/llvm/include/llvm/Analysis/DXILResource.h b/llvm/include/llvm/Analysis/DXILResource.h
index cfeed5c4ef76f..090b6e77eac00 100644
--- a/llvm/include/llvm/Analysis/DXILResource.h
+++ b/llvm/include/llvm/Analysis/DXILResource.h
@@ -670,6 +670,8 @@ class DXILResourceBindingInfo {
       return CBufferSpaces;
     case dxil::ResourceClass::Sampler:
       return SamplerSpaces;
+    default:
+      llvm_unreachable("Invalid resource class");
     }
   }
 

Copy link
Member

@hekota hekota left a comment

Choose a reason for hiding this comment

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

Thank you!

Comment on lines 673 to 674
default:
llvm_unreachable("Invalid resource class");
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't quite right. We should put the llvm_unreachable outside of the switch so that we can still benefit from clang's -Wcovered-switch warnings if dxil::ResourceClass were ever to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call.

@dpaoliello dpaoliello merged commit 5f4dc9b into llvm:main May 9, 2025
8 of 11 checks passed
@dpaoliello dpaoliello deleted the dxil branch May 9, 2025 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:DirectX llvm:analysis Includes value tracking, cost tables and constant folding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants