DSLX is different from Rust for match patterns (currently does equality comparison for any bound identifier)

[meheff]

The motivation is the following (incorrect) use of match:

fn f(x:bool, y:bool, z:bool) -> u32 {
  match true {
    x => u32:42,
    y => u32:43,
    z => u32:44,
    _ => u32:45
  }
}

Superficially this looks like it tests whether arguments x, y, or z are true and return a respective value. However, this construct creates a new temporary named x and binds true to it and returns 42.

This should raise a warning that the match temporaries x, y, and z are unused.

[proppy]

Shouldn't the parser reject this though?

Per https://google.github.io/xls/dslx_reference/#match-expression:

match expressions permit "pattern matching" on data

But here there is no "pattern" to match the data against, so it shouldn't probably not be a valid match expression?

/cc @richmckeever

[proppy]

Note: it does seem useful to have a language construct to encapsulate selection of subexpression given a collection of exclusive bool value; this is effectively https://google.github.io/xls/dslx_std/#priority_sel but what would work w/ any types see #1729.

[cdleary]

Okay discovered this is in a fun space: this is WAI... but it /shouldn't/ be intended to work this way given we'd like to mimic Rust semantics where reasonably possible. I think this was probably my oversight from a long time back, mea culpa.

Currently, in DSLX, if the match pattern identifier is bound in the outer scope, it does an equality comparison (instead of binding the name in the match arm scope) -- the intent of that was to enable things like:

const FOO = ...
const BAR = ...

match x {
  FOO => ...
  BAR => ...
  _ => ...
}

However, that's not the syntax for Rust equality comparison for a match arm -- it's:

const FOO = ...
const BAR = ...

match x {
  x if x == FOO => ... something ...
  x if x == BAR => ... something else ...
  _ => ...
}

So ideally we'd:

• implement support for the suffixed conditional guard
• Make a helper binary for detection and/or migration, maybe gate new parsing behavior behind a feature flag like #[feature(rustic_match)] or something

The good news is this issue caused me to at least write some more unit level tests for warning on unused bindings in studying the behavior from the original example, that PR is incoming. :-)

[cdleary]

As an intermediary step we could write a warning-as-error for any non-const binding that was used for equality comparison in a match, if people think that'd be valuable as a stepping stone it's easy to whip up.

[ericastor]

Note: it does seem useful to have a language construct to encapsulate selection of subexpression given a collection of exclusive bool value; this is effectively https://google.github.io/xls/dslx_std/#priority_sel but what would work w/ any types see #1729.

This sounds like a good enhancement request! A priority_switch construct that follows the first arm with a guard that evaluates to true might be a natural addition, augmenting our existing match construct with something that doesn't re-use the syntax for pattern matching. But if we're following Rust closely, we may not want to add that. I'll leave opinions on that to the DSLX specialists.

[cdleary]

@ericastor @proppy If I'm understanding right, what I've been recommending to people is that if they want this they can do:

match one_hot(sel) {
  0b100 => 
  0b010 => 
  0b001 =>
  _ => fail!("impossible_if_original_was_one_hot", ...)
}

Though the DSLX IR converter will emit a prio selector the one hot guaranteed postcondition will ensure the optimizer turns it into a one hot selector. A match is inherently a priority selector on equality conditions right now as I mentioned in #1909 (comment)

[ericastor]

@cdleary Per this issue's original motivating example, DSLX's current match may be a priority selector on equality conditions - but it's not a priority selector on boolean values, and can in fact end up replacing them with unused bindings. I was suggesting a specialized syntax for ordered boolean conditions, so the author doesn't need to manually concat the conditions all into a single selector (remembering which way around the priority goes), feed that through a one_hot, and then pass that to a match against one-hot binary values.

[cdleary]

@ericastor I think I'm getting confused because the unused binding thing is orthogonal and was the crux of the OP -- now we're talking about expressive power/convenience and what ops things lower to. We do have this ability:

match (x, y, z) {
  (true, _, _) => ...
  (_, true, _) => ...
  (_, _, true) => ...
   _ => ...
}

Are you saying "we could have a more first class syntax for that pattern and/or its similar one-hot matching scenario"?

[ericastor]

@ericastor I think I'm getting confused because the unused binding thing is orthogonal and was the crux of the OP -- now we're talking about expressive power/convenience and what ops things lower to. We do have this ability:

match (x, y, z) {
  (true, _, _) => ...
  (_, true, _) => ...
  (_, _, true) => ...
   _ => ...
}

Are you saying "we could have a more first class syntax for that pattern and/or its similar one-hot matching scenario"?

@cdleary Yes. 😀 Or rather, I'm saying that @proppy's comment that "Note: it does seem useful to have a language construct to encapsulate selection of subexpression given a collection of exclusive bool value" is saying that (EDIT: and endorsing the idea).

[cdleary]

@ericastor Thanks for confirming! I'll break out a separate issue: #1913

[proppy]

Superficially this looks like it tests whether arguments x, y, or z are true and return a respective value. However, this construct creates a new temporary named x and binds true to it and returns 42.

This should raise a warning that the match temporaries x, y, and z are unused.

After doing more testing, I don't think that's the case.

Your example does pass the following test:

fn f(x:bool, y:bool, z:bool) -> u32 {
  match true {
    x => u32:42,
    y => u32:43,
    z => u32:44,
    _ => u32:45
  }
}

#[test]
fn test_f() {
  assert_eq(f(true, false, false), u32:42);
  assert_eq(f(false, true, false), u32:43);
  assert_eq(f(false, false, true), u32:44);  
  assert_eq(f(false, false, false), u32:45);
  assert_eq(f(true, true, true), u32:42);  
}

and produces the following un-optimized IR:

top fn __user_module__f(x: bits[1] id=1, y: bits[1] id=2, z: bits[1] id=3) -> bits[32] {
  literal.4: bits[1] = literal(value=1, id=4, pos=[(0,3,8)])
  eq.9: bits[1] = eq(z, literal.4, id=9)
  eq.7: bits[1] = eq(y, literal.4, id=7)
  eq.5: bits[1] = eq(x, literal.4, id=5)
  concat.13: bits[3] = concat(eq.9, eq.7, eq.5, id=13)
  literal.6: bits[32] = literal(value=42, id=6, pos=[(0,4,9)])
  literal.8: bits[32] = literal(value=43, id=8, pos=[(0,5,9)])
  literal.10: bits[32] = literal(value=44, id=10, pos=[(0,6,9)])
  literal.12: bits[32] = literal(value=45, id=12, pos=[(0,7,9)])
  literal.11: bits[1] = literal(value=1, id=11, pos=[(0,7,4)])
  ret priority_sel.14: bits[32] = priority_sel(concat.13, cases=[literal.6, literal.8, literal.10], default=literal.12, id=14)
}

which seems to be the correct encoding of the original intent (apart from the literal.11 which I'm not quite sure why it's there).

[proppy]

re-reading @cdleary comment:

this is WAI.

Did you mean that the "invalid use" pointed my meheff #1909 (comment) is in fact valid and working as intended (a), or that it is intended to be an "invalid use" (b) ? :)