cranelift-isle: Reject unreachable rules

[jameysharp]

Some of our ISLE rules can never fire because there's a higher-priority rule that will always fire instead.

Sometimes the worst that can happen is we generate sub-optimal output. That's not so bad but we'd still like to know about it so we can fix it. On x64, a current example is that lowering the combination of imul with swiden_high will always emit separate machine instructions for each, rather than a single instruction for the combination.

In other cases there might be instructions which can't be lowered in isolation. If a general rule for lowering one of the instructions is higher-priority than the rule for lowering the combined sequence, then lowering the combined sequence will always fail. I suspect there aren't any of these because somebody probably would have noticed already, but I don't know.

Either way, this is always a bug, so make it a fatal error if we can detect it.

I'm opening this as a draft because I've made this condition a fatal error without fixing any of the existing instances of that error. I'd like help from folks who are more familiar with the backends to figure out how to fix each case, and maybe write new filetests to exercise the relevant rules.

There are currently:

• 5 too-general rules in x64
• 5 too-general rules in s390x (cc: @uweigand)
• 24 too-general rules in aarch64

Details of all these cases are visible in the failure output from CI for this PR.

[github-actions]

Subscribe to Label Action

cc @cfallin, @fitzgen

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 .github/subscribe-to-label.json configuration file.

Learn more.

[uweigand]

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.)

[uweigand]

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.

[jameysharp]

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!

[jameysharp]

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.

[jameysharp]

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

[jameysharp]

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

[elliottt]

Looks good to me, thanks for pulling all the other PRs out!