Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple operations with same opcode #236

Open
p3t3rix opened this issue Jul 7, 2022 · 4 comments
Open

Multiple operations with same opcode #236

p3t3rix opened this issue Jul 7, 2022 · 4 comments

Comments

@p3t3rix
Copy link
Contributor

p3t3rix commented Jul 7, 2022

Currently multiple ops with the same opcode get generated in cases where an extension gets included in KHR extensions.
Example in autgent_context.rs:
5334u32 => Ok(ops::Op::ReportIntersectionNV
5334u32 => Ok(ops::Op::ReportIntersectionKHR

As just ignoring the issue seems out of question (#235 (comment)), we need a different solution to fix this. As currently this prevents the pipeline to be green.

@MarijnS95
Copy link
Collaborator

As just ignoring the issue seems out of question (#235 (comment)) [..] As currently this prevents the pipeline to be green.

To clarify my stance here, the CI being green should imply some form of legitimacy. If we start ignoring sensible errors - even if they may seem rather harmless on the surface - a green CI becomes more and more meaningless in my eyes.

Again these warnings aren't the end of the world and I haven't investigated whether they could really be harmful, they are at least confusing.

For example:

5334u32 => Ok(ops::Op::ReportIntersectionNV
5334u32 => Ok(ops::Op::ReportIntersectionKHR

Picks and returns the NV variant over KHR whereas the NV extensions shouldn't be used anymore since the standardization through KHR.

@MarijnS95
Copy link
Collaborator

MarijnS95 commented Jul 8, 2022

This was already thought of earlier 😬:

rspirv/autogen/src/sr.rs

Lines 537 to 540 in 4f408eb

// TODO: filter duplicated symbols mapping to the same discriminator to avoid
// unreachable patterns.
#[allow(unreachable_patterns)]
pub fn lift_op(

(Comment specifically on fn lift_op(), which is the function giving off these warnings)

@msiglreith
Copy link
Contributor

One more occurrence for reference

if instruction.opname.starts_with("OpType")
// Ignore AccelerationStructureNV which has the same opcode as AccelerationStructureKHR
&& instruction.opname != "OpTypeAccelerationStructureNV"
{

@MarijnS95
Copy link
Collaborator

MarijnS95 commented Jul 8, 2022

👍

It's funny because identical warnings are already ignored by clippy::unreachable_patterns, and different ones now show up as clippy::match_overlapping_arm (which is surprisingly not mentioned in the title/commit message).

At the same time some #[allow(clippy::unreachable_patterns)] don't trigger any warnings currently...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants