Skip to content

[DSLX:ir_conv] IR convert trailing match arms w/ multiple patterns.#1921

Merged
copybara-service[bot] merged 1 commit intogoogle:mainfrom
xlsynth:cdleary/2025-02-08-multi-pattern-trailing-arm
Feb 18, 2025
Merged

[DSLX:ir_conv] IR convert trailing match arms w/ multiple patterns.#1921
copybara-service[bot] merged 1 commit intogoogle:mainfrom
xlsynth:cdleary/2025-02-08-multi-pattern-trailing-arm

Conversation

@cdleary
Copy link
Collaborator

@cdleary cdleary commented Feb 9, 2025

Previously we would give an IR conversion error, which was a vestige from where we needed the trailing arm to be a wildcard. Now with exhaustiveness we know the trailing match arm must be usable as a default case in the priority select that we lower to.

@cdleary cdleary marked this pull request as ready for review February 9, 2025 18:17
@grebe grebe requested a review from richmckeever February 11, 2025 00:55
@cdleary cdleary force-pushed the cdleary/2025-02-08-multi-pattern-trailing-arm branch from f8c7e9c to 8804cca Compare February 14, 2025 22:22
@cdleary
Copy link
Collaborator Author

cdleary commented Feb 14, 2025

Rebased this, PTAL.

@copybara-service copybara-service bot merged commit 6f68726 into google:main Feb 18, 2025
6 checks passed
@proppy
Copy link
Member

proppy commented Feb 18, 2025

@cdleary do we want to also update the existing examples to remove unnecessary trailing match clause w/ wildcards?

@cdleary
Copy link
Collaborator Author

cdleary commented Feb 18, 2025

@proppy Yeah I'd think so, can't think of a reason why not -- I did a quick search for .md that have _ => and nothing was jumping out at me, do you see some I should have updated but missed?

@proppy
Copy link
Member

proppy commented Feb 18, 2025

found at least one :)

_ => fail!("invalid_column_index", s32:0)

but I was also wondering if we should make this a failure/warning during conversion?

@cdleary
Copy link
Collaborator Author

cdleary commented Feb 24, 2025

found at least one :)

_ => fail!("invalid_column_index", s32:0)

but I was also wondering if we should make this a failure/warning during conversion?

@proppy This is a warning https://sourcegraph.com/github.com/google/xls/-/blob/xls/dslx/warning_kind.h?L45 already-exhaustive-match , it's just disabled by default so Google-internal codebase can do the global change that enables it by default. In my repo I have a toolchain-level config that enables it by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants