From bc0de464bc4de884dd403b06883ca546ab072b31 Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Wed, 24 Nov 2021 10:42:58 -0800 Subject: [PATCH] Update aarch64 backend's ISLE code to be rule-ordering-independent. In [this comment](https://github.com/bytecodealliance/wasmtime/pull/3545#discussion_r756284757) I noted a potential subtle issue with the way that a few rules were written that is fine now but could cause some unexpected pain when we get around to verification. Specifically, a set of rules of the form ``` (rule (A (B _)) (C)) (rule (A _) (D)) ``` should, under any reasonable "default" rule ordering scheme, fire the more specific rule `(A (B _))` when applicable, in preference to the second "fallback" rule. However, for future verification-specific applications of ISLE, we want to ensure the property that a rule's meaning/validity is not dependent on being overridden by more specific rules. In other words, if a rule specifies a rewrite, that rewrite should always be correct; and choosing a more specific rule can give a *better* compilation (better generated code) but should not be necessary for correctness. This is an admittedly under-documented part of the language, though in the pending #3560 I added a note about rule ordering being a heuristic that should hopefully make this slightly clearer. Ultimately I want to have tests that choose non-default rule orderings and differentially fuzz in order to be sure that we're following this principle; and of course once we're actually doing verification, we'll catch issues like this upfront. Apologies for the subtle footgun here and hopefully the reasoning is clear enough :-) --- cranelift/codegen/src/isa/aarch64/lower.isle | 6 ++--- .../lower/isle/generated_code.manifest | 2 +- .../isa/aarch64/lower/isle/generated_code.rs | 27 ++++++++++++------- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/cranelift/codegen/src/isa/aarch64/lower.isle b/cranelift/codegen/src/isa/aarch64/lower.isle index 2c8c8a8ea0..df8713a02a 100644 --- a/cranelift/codegen/src/isa/aarch64/lower.isle +++ b/cranelift/codegen/src/isa/aarch64/lower.isle @@ -73,17 +73,17 @@ ;; Helper to use either a 32 or 64-bit add depending on the input type. (decl iadd_op (Type) ALUOp) (rule (iadd_op (fits_in_32 _ty)) (ALUOp.Add32)) -(rule (iadd_op _ty) (ALUOp.Add64)) +(rule (iadd_op $I64) (ALUOp.Add64)) ;; Helper to use either a 32 or 64-bit sub depending on the input type. (decl isub_op (Type) ALUOp) (rule (isub_op (fits_in_32 _ty)) (ALUOp.Sub32)) -(rule (isub_op _ty) (ALUOp.Sub64)) +(rule (isub_op $I64) (ALUOp.Sub64)) ;; Helper to use either a 32 or 64-bit madd depending on the input type. (decl madd_op (Type) ALUOp3) (rule (madd_op (fits_in_32 _ty)) (ALUOp3.MAdd32)) -(rule (madd_op _ty) (ALUOp3.MAdd64)) +(rule (madd_op $I64) (ALUOp3.MAdd64)) ;; vectors diff --git a/cranelift/codegen/src/isa/aarch64/lower/isle/generated_code.manifest b/cranelift/codegen/src/isa/aarch64/lower/isle/generated_code.manifest index 202f798ec0..7452e9507e 100644 --- a/cranelift/codegen/src/isa/aarch64/lower/isle/generated_code.manifest +++ b/cranelift/codegen/src/isa/aarch64/lower/isle/generated_code.manifest @@ -1,4 +1,4 @@ src/clif.isle 9c0563583e5500de00ec5e226edc0547ac3ea789c8d76f1da0401c80ec619320fdc9a6f17fd76bbcac74a5894f85385c1f51c900c2b83bc9906d03d0f29bf5cb src/prelude.isle e4933f2bcb6cd9e00cb6dc0c47c43d096d0c4e37468af17a38fad8906b864d975e0a8b98d15c6a5e2bccf255ec2ced2466991c3405533e9cafefbf4d9ac46823 src/isa/aarch64/inst.isle 67a43022bb2e0b8ae06b71c7c49f9b9997a9c6ca109e35f5018b9cd64105a0fe8b103943fb34ca7da45cea9db7327e00954e88606845d5ebc370bc6c3045a04f -src/isa/aarch64/lower.isle efb81eb92d8ab0a408e0de222825f632b40d7c91b0bf79e3144f7d76f0d1c743bb505ab991723363cab504a0f95b18232811f1519ea1d8c15d2113be166f590c +src/isa/aarch64/lower.isle f3699bd266aa0fe5389ace7d4e6e79b7f8778e61cd803b564af8508541b8e3c3235431a3bb83bbec46cdbc92236fd84b0ad290a276ff34d24129a8b3aa54ad0d diff --git a/cranelift/codegen/src/isa/aarch64/lower/isle/generated_code.rs b/cranelift/codegen/src/isa/aarch64/lower/isle/generated_code.rs index f842978241..04209b4573 100644 --- a/cranelift/codegen/src/isa/aarch64/lower/isle/generated_code.rs +++ b/cranelift/codegen/src/isa/aarch64/lower/isle/generated_code.rs @@ -2046,38 +2046,47 @@ pub fn constructor_lower(ctx: &mut C, arg0: Inst) -> Option(ctx: &mut C, arg0: Type) -> Option { let pattern0_0 = arg0; + if pattern0_0 == I64 { + // Rule at src/isa/aarch64/lower.isle line 76. + let expr0_0 = ALUOp::Add64; + return Some(expr0_0); + } if let Some(pattern1_0) = C::fits_in_32(ctx, pattern0_0) { // Rule at src/isa/aarch64/lower.isle line 75. let expr0_0 = ALUOp::Add32; return Some(expr0_0); } - // Rule at src/isa/aarch64/lower.isle line 76. - let expr0_0 = ALUOp::Add64; - return Some(expr0_0); + return None; } // Generated as internal constructor for term isub_op. pub fn constructor_isub_op(ctx: &mut C, arg0: Type) -> Option { let pattern0_0 = arg0; + if pattern0_0 == I64 { + // Rule at src/isa/aarch64/lower.isle line 81. + let expr0_0 = ALUOp::Sub64; + return Some(expr0_0); + } if let Some(pattern1_0) = C::fits_in_32(ctx, pattern0_0) { // Rule at src/isa/aarch64/lower.isle line 80. let expr0_0 = ALUOp::Sub32; return Some(expr0_0); } - // Rule at src/isa/aarch64/lower.isle line 81. - let expr0_0 = ALUOp::Sub64; - return Some(expr0_0); + return None; } // Generated as internal constructor for term madd_op. pub fn constructor_madd_op(ctx: &mut C, arg0: Type) -> Option { let pattern0_0 = arg0; + if pattern0_0 == I64 { + // Rule at src/isa/aarch64/lower.isle line 86. + let expr0_0 = ALUOp3::MAdd64; + return Some(expr0_0); + } if let Some(pattern1_0) = C::fits_in_32(ctx, pattern0_0) { // Rule at src/isa/aarch64/lower.isle line 85. let expr0_0 = ALUOp3::MAdd32; return Some(expr0_0); } - // Rule at src/isa/aarch64/lower.isle line 86. - let expr0_0 = ALUOp3::MAdd64; - return Some(expr0_0); + return None; }