Stream: git-wasmtime

Topic: wasmtime / PR #14295 Fix subtle ISLE extractor issue lead...

Wasmtime GitHub notifications bot (Sep 08 2026 at 00:18):

cfallin opened PR #14295 from cfallin:fix-second-result-etor to bytecodealliance:main:

In #14293, a test case that uses the first result (i.e., the product) of an smul_overflow operator as a condition (e.g. as part of icmp eq comparing to zero) incorrectly triggers the lowering rule I added in #14254 which was meant to match only compare-to-zero on the second (overflow) result.

This was a result if a fairly subtle issue involving auto-conversions in ISLE. I had written

(rule (is_nonzero (second_result umul @ (smul_overflow ...)))
      ...)

where the intent was to match an is_nonzero (which is a helper term) lowering with the second result (overflow flag) of the smul_overflow.

second_result has a term signature (Value) Inst, in other words it takes an Inst and returns an Option<Value>. is_nonzero takes a Value. So we auto-convert the Value in the first arg position of is_nonzer to an Inst; that uses def_inst, which looks up the defining instruction of the given value. Then second_result takes that Inst and gives the second value. But then the next level, (smul_overflow ...), *again* uses def_inst and goes from the (second result) Value` back to the inst and matches.

In other words, we're too permissive with the autoconversions on Value to Inst; all of this was designed at a time when we more or less only handled single-result instructions with any nontrivial lowering rule, so the two were mostly interchangeable. The handling for the overflow-flag ops changes that.

The specific step in that chain above that is unambiguously wrong wrt intent is (first result) Value -> Inst -> second_result matching. So this PR instead introduces is_second_result that is Value -> Option<Value> and matches only when the specific Value is the second result of an instruction.

This does have me thinking a bit more about the role that the Value -> Inst autoconvert matching plays. It is absolutely essential to the ergonomics of ISLE: without it, we couldn't write

(rule (lower (iadd (imul a b) c)) ...)

because iadd's args are Values and we need to match back to an Inst for imul. But we also have cases like the one in this PR where we really shouldn't be so permissive. Perhaps we want a kind of type modifier (=Value ?) that means "exactly this type, not autoconverted". I'll bring this up in the Cranelift meeting this week.

Fixes #14293.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please review the Bytecode Alliance's AI tool usage policy at
https://github.com/bytecodealliance/governance/blob/main/AI_TOOL_POLICY.md

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Sep 08 2026 at 00:18):

cfallin requested alexcrichton for a review on PR #14295.

Wasmtime GitHub notifications bot (Sep 08 2026 at 00:18):

cfallin requested wasmtime-compiler-reviewers for a review on PR #14295.

Wasmtime GitHub notifications bot (Sep 08 2026 at 00:18):

cfallin edited PR #14295:

In #14293, a test case that uses the first result (i.e., the product) of an smul_overflow operator as a condition (e.g. as part of icmp eq comparing to zero) incorrectly triggers the lowering rule I added in #14254 which was meant to match only compare-to-zero on the second (overflow) result.

This was a result of a fairly subtle issue involving auto-conversions in ISLE. I had written

(rule (is_nonzero (second_result umul @ (smul_overflow ...)))
      ...)

where the intent was to match an is_nonzero (which is a helper term) lowering with the second result (overflow flag) of the smul_overflow.

second_result has a term signature (Value) Inst, in other words it takes an Inst and returns an Option<Value>. is_nonzero takes a Value. So we auto-convert the Value in the first arg position of is_nonzer to an Inst; that uses def_inst, which looks up the defining instruction of the given value. Then second_result takes that Inst and gives the second value. But then the next level, (smul_overflow ...), *again* uses def_inst and goes from the (second result) Value` back to the inst and matches.

In other words, we're too permissive with the autoconversions on Value to Inst; all of this was designed at a time when we more or less only handled single-result instructions with any nontrivial lowering rule, so the two were mostly interchangeable. The handling for the overflow-flag ops changes that.

The specific step in that chain above that is unambiguously wrong wrt intent is (first result) Value -> Inst -> second_result matching. So this PR instead introduces is_second_result that is Value -> Option<Value> and matches only when the specific Value is the second result of an instruction.

This does have me thinking a bit more about the role that the Value -> Inst autoconvert matching plays. It is absolutely essential to the ergonomics of ISLE: without it, we couldn't write

(rule (lower (iadd (imul a b) c)) ...)

because iadd's args are Values and we need to match back to an Inst for imul. But we also have cases like the one in this PR where we really shouldn't be so permissive. Perhaps we want a kind of type modifier (=Value ?) that means "exactly this type, not autoconverted". I'll bring this up in the Cranelift meeting this week.

Fixes #14293.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please review the Bytecode Alliance's AI tool usage policy at
https://github.com/bytecodealliance/governance/blob/main/AI_TOOL_POLICY.md

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Sep 08 2026 at 00:19):

cfallin edited PR #14295:

In #14293, a test case that uses the first result (i.e., the product) of an smul_overflow operator as a condition (e.g. as part of icmp eq comparing to zero) incorrectly triggers the lowering rule I added in #14254 which was meant to match only compare-to-zero on the second (overflow) result.

This was a result of a fairly subtle issue involving auto-conversions in ISLE. I had written

(rule (is_nonzero (second_result umul @ (smul_overflow ...)))
      ...)

where the intent was to match an is_nonzero (which is a helper term) lowering with the second result (overflow flag) of the smul_overflow.

second_result has a term signature (Value) Inst, in other words it takes an Inst and returns an Option<Value>. is_nonzero takes a Value. So we auto-convert the Value in the first arg position of is_nonzer to an Inst; that uses def_inst, which looks up the defining instruction of the given value. Then second_result takes that Inst and gives the second value. But then the next level, (smul_overflow ...), again uses def_inst and goes from the (second result) Value back to the inst and matches.

In other words, we're too permissive with the autoconversions on Value to Inst; all of this was designed at a time when we more or less only handled single-result instructions with any nontrivial lowering rule, so the two were mostly interchangeable. The handling for the overflow-flag ops changes that.

The specific step in that chain above that is unambiguously wrong wrt intent is (first result) Value -> Inst -> second_result matching. So this PR instead introduces is_second_result that is Value -> Option<Value> and matches only when the specific Value is the second result of an instruction.

This does have me thinking a bit more about the role that the Value -> Inst autoconvert matching plays. It is absolutely essential to the ergonomics of ISLE: without it, we couldn't write

(rule (lower (iadd (imul a b) c)) ...)

because iadd's args are Values and we need to match back to an Inst for imul. But we also have cases like the one in this PR where we really shouldn't be so permissive. Perhaps we want a kind of type modifier (=Value ?) that means "exactly this type, not autoconverted". I'll bring this up in the Cranelift meeting this week.

Fixes #14293.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please review the Bytecode Alliance's AI tool usage policy at
https://github.com/bytecodealliance/governance/blob/main/AI_TOOL_POLICY.md

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Sep 08 2026 at 01:54):

github-actions[bot] added the label cranelift on PR #14295.

Wasmtime GitHub notifications bot (Sep 08 2026 at 01:54):

github-actions[bot] added the label cranelift:area:aarch64 on PR #14295.

Wasmtime GitHub notifications bot (Sep 08 2026 at 01:54):

github-actions[bot] added the label cranelift:area:machinst on PR #14295.

Wasmtime GitHub notifications bot (Sep 08 2026 at 01:54):

github-actions[bot] added the label cranelift:area:x64 on PR #14295.

Wasmtime GitHub notifications bot (Sep 08 2026 at 14:30):

alexcrichton commented on PR #14295:

I don't really understand what's going on here, and I also didn't catch this in prior review, so I'm going to swap in @fitzgen

Wasmtime GitHub notifications bot (Sep 08 2026 at 14:30):

alexcrichton unassigned alexcrichton from PR #14295 Fix subtle ISLE extractor issue leading to incorrect matching in *mul_overflow lowering..

Wasmtime GitHub notifications bot (Sep 08 2026 at 14:30):

alexcrichton requested fitzgen for a review on PR #14295.

Wasmtime GitHub notifications bot (Sep 08 2026 at 20:01):

:thumbs_up: fitzgen submitted PR review:

LGTM modulo comments below

Wasmtime GitHub notifications bot (Sep 08 2026 at 20:01):

:speech_balloon: fitzgen created PR review comment:

Seems like we could probably minimize this test case a bit

Wasmtime GitHub notifications bot (Sep 08 2026 at 20:01):

:speech_balloon: fitzgen created PR review comment:

Do we still even want to have second_result? Is anything still using it that couldn't move to is_second_result?

Wasmtime GitHub notifications bot (Sep 08 2026 at 20:01):

:speech_balloon: fitzgen created PR review comment:

self.lower_ctx.dfg().inst_results(inst).get(1) == Some(&val) is a little shorter and more clear to me

Wasmtime GitHub notifications bot (Sep 08 2026 at 23:24):

:memo: cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 08 2026 at 23:24):

:speech_balloon: cfallin created PR review comment:

Yes, it's still used elsewhere, specifically to get the Value for the second result from the actual Inst in lower (where we have a bound Inst, not Value) on uadd_overflow. Will leave for now but happy to consider refactors in followup along with the auto-conversion question.

Wasmtime GitHub notifications bot (Sep 08 2026 at 23:33):

cfallin updated PR #14295.

Wasmtime GitHub notifications bot (Sep 08 2026 at 23:33):

:memo: cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 08 2026 at 23:33):

:speech_balloon: cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Sep 08 2026 at 23:34):

:memo: cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 08 2026 at 23:34):

:speech_balloon: cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Sep 08 2026 at 23:34):

cfallin has enabled auto merge for PR #14295.

Wasmtime GitHub notifications bot (Sep 08 2026 at 23:52):

cfallin added PR #14295 Fix subtle ISLE extractor issue leading to incorrect matching in *mul_overflow lowering. to the merge queue

Wasmtime GitHub notifications bot (Sep 09 2026 at 00:19):

:check: cfallin merged PR #14295.

Wasmtime GitHub notifications bot (Sep 09 2026 at 00:19):

cfallin removed PR #14295 Fix subtle ISLE extractor issue leading to incorrect matching in *mul_overflow lowering. from the merge queue

Last updated: Sep 20 2026 at 18:08 UTC