wasmtime / issue #5322 cranelift-isle: Reject unreachable... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #5322 cranelift-isle: Reject unreachable...

Wasmtime GitHub notifications bot (Nov 24 2022 at 02:44):

github-actions[bot] commented on issue #5322:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:docs", "isle"

Thus the following users have been cc'd because of the following labels:

cfallin: isle

fitzgen: isle

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Nov 24 2022 at 14:43):

uweigand commented on issue #5322:

Most of the s390x instances appear to be of the same pattern:
;; Load scalar value from general-purpose register.
(rule 1 (lower (has_type ty (scalar_to_vector
                             x @ (value_type in_ty))))
      (if (ty_int_ref_scalar_64 in_ty))
      (vec_insert_lane ty (vec_imm ty 0) x (be_lane_idx ty 0) (zero_reg)))

;; Load scalar value from floating-point register.
(rule 0 (lower (has_type ty (scalar_to_vector
                             x @ (value_type (ty_scalar_float _)))))
      (vec_move_lane_and_zero ty (be_lane_idx ty 0) x 0))
where it says that the rule with higher priority will always shadow the rule with lower priority. But that's not actually the case because the first rule has an if that should prevent it from firing, and in that case the second rule may apply.

Am I mistaken in my interpretation here, or does the checker not consider if here?

(Of course, in these instances, the rules can be rewritten without if as well. Still, I think the checker should handle if.)

Wasmtime GitHub notifications bot (Nov 24 2022 at 14:47):

uweigand commented on issue #5322:

The remaining s390x case also seems a false positive. The higher-priority rule is:
(rule 3 (vec_imm_splat (multi_lane 16 _) (u32_pair _ (u16_pair _ (u8_pair n n))))
      (vec_imm_splat $I8X16 (u8_as_u64 n)))
and the lower-priority rule is:
(rule 2 (vec_imm_splat ty @ (multi_lane 16 _) n)
      (vec_imm_replicate ty (u64_as_i16 n)))
But the higher-priority rule does not always fire, it requires a match of the two operands of the inner u8_pair. I.e. it only matches on constants N where (N >> 8) & 0xFF == N & 0xFF.

Wasmtime GitHub notifications bot (Nov 27 2022 at 20:32):

jameysharp commented on issue #5322:

It should be handling if correctly. I believe the trouble with the first case is that I'm not handling pure (and therefore fallible) constructors like ty_int_ref_scalar_64 correctly. As for the second, I've definitely had a hard time reasoning about equality constraints; I thought I had fixed the false positives I previously encountered with those, but apparently not.

I'll look into both issues tomorrow. Thanks for digging into these!

Wasmtime GitHub notifications bot (Nov 28 2022 at 18:45):

jameysharp commented on issue #5322:

Fixing the false positive for vec_imm_splat was easy. That didn't eliminate any other errors in any backend, so that was apparently the only false positive of that form.

Fixing the other issues is turning out to be more of a pain. Naively adding constraints for all uses of fallible constructors effectively disables all overlap checking, because constructors on the right-hand side of the rule shouldn't count, and by convention all our lowerings delegate to other internal constructors. But I think I have a workable plan.

Wasmtime GitHub notifications bot (Nov 28 2022 at 19:51):

jameysharp commented on issue #5322:

Now that I've fixed pure constructors, there are no errors left in s390x. Thanks @uweigand! That also fixed most of the aarch64 errors and some of the x64 errors. The new failure output from CI shows:

3 over-general rules in x64, all in the implementation of imul on vectors (cc: @elliottt)

2 over-general rules in aarch64

Wasmtime GitHub notifications bot (Nov 29 2022 at 02:04):

jameysharp commented on issue #5322:

For review and history purposes, I've extracted the various bug fixes and cleanups as #5337 and #5338. Once #5337 lands I'll rebase bf51b793d0a9065fbb53a12312d7b399189972e2 from this PR on it to start enforcing the unreachable-rule check. So on this PR, please review only that commit.

Last updated: Nov 22 2024 at 16:03 UTC