Repository: bytecodealliance/wasmtime
Pull request #5810 (2023-02-16 21:57:57)

Handle failures in the `determine` step
I saw some PRs fail this step earlier today due to rate limits but it ended up not failing the entire PR's CI due to it not being listed in the final set of dependencies, so add it there.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

- [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
- [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
- [ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the [code of
conduct](https://github.com/bytecodealliance/wasmtime/blob/master/CODE_OF_CONDUCT.md).
-->



Patch:
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 09613f407..07008be3f 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -448,7 +448,7 @@ jobs:
     - run: |
         sudo apt-get update && sudo apt-get install -y gdb lldb llvm
         cargo test test_debug_dwarf -- --ignored --test-threads 1
-      if: matrix.os == 'ubuntu-latest' && matrix.target == ''
+      if: matrix.os == 'ubuntu-latest' && matrix.target == ''&& needs.determine.outputs.run-full
       env:
         RUST_BACKTRACE: 1
 
@@ -679,6 +679,7 @@ jobs:
       - bench
       - meta_deterministic_check
       - verify-publish
+      - determine
     if: always()
     steps:
     - name: Dump needs context
diff --git a/.github/workflows/triage.yml b/.github/workflows/triage.yml
index 4da009f8b..c7bde9931 100644
--- a/.github/workflows/triage.yml
+++ b/.github/workflows/triage.yml
@@ -4,11 +4,11 @@ on:
   issues:
     types: ["labeled"]
   schedule:
-    # Run pull request triage every 5 minutes. Ideally, this would be on
+    # Run pull request triage once an hour. Ideally, this would be on
     # "labeled" types of pull request events, but that doesn't work if the pull
     # request is from another fork. For example, see
     # https://github.com/actions/labeler/issues/12
-    - cron: '*/5 * * * *'
+    - cron: '42 * * * *'
 
 concurrency:
   group: issue-triage
diff --git a/cranelift/codegen/src/egraph.rs b/cranelift/codegen/src/egraph.rs
index 02bfeb013..2cbe412e3 100644
--- a/cranelift/codegen/src/egraph.rs
+++ b/cranelift/codegen/src/egraph.rs
@@ -7,7 +7,7 @@ use crate::dominator_tree::DominatorTree;
 use crate::egraph::domtree::DomTreeWithChildren;
 use crate::egraph::elaborate::Elaborator;
 use crate::fx::FxHashSet;
-use crate::inst_predicates::{is_mergeable_for_egraph, is_pure_for_egraph};
+use crate::inst_predicates::is_pure_for_egraph;
 use crate::ir::{
     DataFlowGraph, Function, Inst, InstructionData, Type, Value, ValueDef, ValueListPool,
 };
@@ -285,41 +285,10 @@ where
     fn optimize_skeleton_inst(&mut self, inst: Inst) -> bool {
         self.stats.skeleton_inst += 1;
 
-        // First, can we try to deduplicate? We need to keep some copy
-        // of the instruction around because it's side-effecting, but
-        // we may be able to reuse an earlier instance of it.
-        if is_mergeable_for_egraph(self.func, inst) {
-            let result = self.func.dfg.inst_results(inst)[0];
-            trace!(" -> mergeable side-effecting op {}", inst);
-            let inst = NewOrExistingInst::Existing(inst);
-            let gvn_context = GVNContext {
-                union_find: self.eclasses,
-                value_lists: &self.func.dfg.value_lists,
-            };
-
-            // Does this instruction already exist? If so, add entries to
-            // the value-map to rewrite uses of its results to the results
-            // of the original (existing) instruction. If not, optimize
-            // the new instruction.
-            let key = inst.get_inst_key(&self.func.dfg);
-            if let Some(&orig_result) = self.gvn_map.get(&key, &gvn_context) {
-                // Hit in GVN map -- reuse value.
-                self.value_to_opt_value[result] = orig_result;
-                self.eclasses.union(orig_result, result);
-                trace!(" -> merges result {} to {}", result, orig_result);
-                true
-            } else {
-                // Otherwise, insert it into the value-map.
-                self.value_to_opt_value[result] = result;
-                self.gvn_map.insert(key, result, &gvn_context);
-                trace!(" -> inserts as new (no GVN)");
-                false
-            }
-        }
-        // Otherwise, if a load or store, process it with the alias
-        // analysis to see if we can optimize it (rewrite in terms of
-        // an earlier load or stored value).
-        else if let Some(new_result) =
+        // If a load or store, process it with the alias analysis to see
+        // if we can optimize it (rewrite in terms of an earlier load or
+        // stored value).
+        if let Some(new_result) =
             self.alias_analysis
                 .process_inst(self.func, self.alias_analysis_state, inst)
         {
diff --git a/cranelift/codegen/src/inst_predicates.rs b/cranelift/codegen/src/inst_predicates.rs
index f82f41daa..9b284e8b8 100644
--- a/cranelift/codegen/src/inst_predicates.rs
+++ b/cranelift/codegen/src/inst_predicates.rs
@@ -73,25 +73,6 @@ pub fn is_pure_for_egraph(func: &Function, inst: Inst) -> bool {
     has_one_result && (is_readonly_load || (!op.can_load() && !trivially_has_side_effects(op)))
 }
 
-/// Can the given instruction be merged into another copy of itself?
-/// These instructions may have side-effects, but as long as we retain
-/// the first instance of the instruction, the second and further
-/// instances are redundant if they would produce the same trap or
-/// result.
-pub fn is_mergeable_for_egraph(func: &Function, inst: Inst) -> bool {
-    let op = func.dfg.insts[inst].opcode();
-    // We can only merge one-result operators due to the way that GVN
-    // is structured in the egraph implementation.
-    let has_one_result = func.dfg.inst_results(inst).len() == 1;
-    has_one_result
-        // Loads/stores are handled by alias analysis and not
-        // otherwise mergeable.
-        && !op.can_load()
-        && !op.can_store()
-        // Can only have idempotent side-effects.
-        && (!has_side_effect(func, inst) || op.side_effects_idempotent())
-}
-
 /// Does the given instruction have any side-effect as per [has_side_effect], or else is a load,
 /// but not the get_pinned_reg opcode?
 pub fn has_lowering_side_effect(func: &Function, inst: Inst) -> bool {
diff --git a/cranelift/codegen/src/machinst/lower.rs b/cranelift/codegen/src/machinst/lower.rs
index 5115c5b01..c199aa15a 100644
--- a/cranelift/codegen/src/machinst/lower.rs
+++ b/cranelift/codegen/src/machinst/lower.rs
@@ -10,8 +10,8 @@ use crate::fx::{FxHashMap, FxHashSet};
 use crate::inst_predicates::{has_lowering_side_effect, is_constant_64bit};
 use crate::ir::{
     ArgumentPurpose, Block, Constant, ConstantData, DataFlowGraph, ExternalName, Function,
-    GlobalValue, GlobalValueData, Immediate, Inst, InstructionData, MemFlags, Opcode, RelSourceLoc,
-    Type, Value, ValueDef, ValueLabelAssignments, ValueLabelStart,
+    GlobalValue, GlobalValueData, Immediate, Inst, InstructionData, MemFlags, RelSourceLoc, Type,
+    Value, ValueDef, ValueLabelAssignments, ValueLabelStart,
 };
 use crate::machinst::{
     writable_value_regs, BlockIndex, BlockLoweringOrder, Callee, LoweredBlock, MachLabel, Reg,
@@ -898,39 +898,29 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
         bindex: BlockIndex,
         // Original CLIF block:
         block: Block,
-        branches: &SmallVec<[Inst; 2]>,
-        targets: &SmallVec<[MachLabel; 2]>,
+        branch: Inst,
+        targets: &[MachLabel],
     ) -> CodegenResult<()> {
         trace!(
-            "lower_clif_branches: block {} branches {:?} targets {:?}",
+            "lower_clif_branches: block {} branch {:?} targets {:?}",
             block,
-            branches,
+            branch,
             targets,
         );
-        // A block should end with at most two branches. The first may be a
-        // conditional branch; a conditional branch can be followed only by an
-        // unconditional branch or fallthrough. Otherwise, if only one branch,
-        // it may be an unconditional branch, a fallthrough, a return, or a
-        // trap. These conditions are verified by `is_block_basic()` during the
-        // verifier pass.
-        assert!(branches.len() <= 2);
-        if branches.len() == 2 {
-            assert!(self.data(branches[1]).opcode() == Opcode::Jump);
-        }
         // When considering code-motion opportunities, consider the current
-        // program point to be the first branch.
-        self.cur_inst = Some(branches[0]);
-        // Lower the first branch in ISLE.  This will automatically handle
-        // the second branch (if any) by emitting a two-way conditional branch.
+        // program point to be this branch.
+        self.cur_inst = Some(branch);
+
+        // Lower the branch in ISLE.
         backend
-            .lower_branch(self, branches[0], targets)
+            .lower_branch(self, branch, targets)
             .unwrap_or_else(|| {
                 panic!(
                     "should be implemented in ISLE: branch = `{}`",
-                    self.f.dfg.display_inst(branches[0]),
+                    self.f.dfg.display_inst(branch),
                 )
             });
-        let loc = self.srcloc(branches[0]);
+        let loc = self.srcloc(branch);
         self.finish_ir_inst(loc);
         // Add block param outputs for current block.
         self.lower_branch_blockparam_args(bindex);
@@ -967,26 +957,20 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
         &self,
         bindex: BlockIndex,
         _bb: Block,
-        branches: &mut SmallVec<[Inst; 2]>,
         targets: &mut SmallVec<[MachLabel; 2]>,
-    ) {
-        branches.clear();
+    ) -> Option<Inst> {
         targets.clear();
         let mut last_inst = None;
         for &(inst, succ) in self.vcode.block_order().succ_indices(bindex) {
-            // Avoid duplicates: this ensures a br_table is only inserted once.
-            if last_inst != Some(inst) {
-                branches.push(inst);
-            } else {
-                debug_assert!(
-                    self.f.dfg.insts[inst].opcode() == Opcode::BrTable
-                        || self.f.dfg.insts[inst].opcode() == Opcode::Brif
-                );
-                debug_assert!(branches.len() == 1);
-            }
+            // Basic blocks may end in a single branch instruction, but those instructions may have
+            // multiple destinations. As such, all `inst` values in `succ_indices` must be the
+            // same, or this basic block would have multiple branch instructions present.
+            debug_assert!(last_inst.map_or(true, |prev| prev == inst));
             last_inst = Some(inst);
             targets.push(MachLabel::from_block(succ));
         }
+
+        last_inst
     }
 
     /// Lower the function.
@@ -1010,7 +994,6 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
         self.vcode.set_entry(BlockIndex::new(0));
 
         // Reused vectors for branch lowering.
-        let mut branches: SmallVec<[Inst; 2]> = SmallVec::new();
         let mut targets: SmallVec<[MachLabel; 2]> = SmallVec::new();
 
         // get a copy of the lowered order; we hold this separately because we
@@ -1032,10 +1015,9 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
 
             // End branches.
             if let Some(bb) = lb.orig_block() {
-                self.collect_branches_and_targets(bindex, bb, &mut branches, &mut targets);
-                if branches.len() > 0 {
-                    self.lower_clif_branches(backend, bindex, bb, &branches, &targets)?;
-                    self.finish_ir_inst(self.srcloc(branches[0]));
+                if let Some(branch) = self.collect_branches_and_targets(bindex, bb, &mut targets) {
+                    self.lower_clif_branches(backend, bindex, bb, branch, &targets)?;
+                    self.finish_ir_inst(self.srcloc(branch));
                 }
             } else {
                 // If no orig block, this must be a pure edge block;


Test patch:
diff --git a/ci/build-test-matrix.js b/ci/build-test-matrix.js
index c02752c5d..d0dd51b93 100644
--- a/ci/build-test-matrix.js
+++ b/ci/build-test-matrix.js
@@ -36,7 +36,8 @@ const array = [
   {
     "os": "ubuntu-latest",
     "name": "Test Linux x86_64",
-    "filter": "linux-x64"
+    "filter": "linux-x64",
+    "isa": "x64"
   },
   {
     "os": "macos-latest",
diff --git a/cranelift/filetests/filetests/wasm/duplicate-loads-dynamic-memory-egraph.wat b/cranelift/filetests/filetests/wasm/duplicate-loads-dynamic-memory-egraph.wat
deleted file mode 100644
index b6d4e88dc..000000000
--- a/cranelift/filetests/filetests/wasm/duplicate-loads-dynamic-memory-egraph.wat
+++ /dev/null
@@ -1,92 +0,0 @@
-;;! target = "x86_64"
-;;!
-;;! optimize = true
-;;!
-;;! settings = [
-;;!   "enable_heap_access_spectre_mitigation=true",
-;;!   "opt_level=speed_and_size",
-;;!   "use_egraphs=true"
-;;! ]
-;;!
-;;! [globals.vmctx]
-;;! type = "i64"
-;;! vmctx = true
-;;!
-;;! [globals.heap_base]
-;;! type = "i64"
-;;! load = { base = "vmctx", offset = 0 }
-;;!
-;;! [globals.heap_bound]
-;;! type = "i64"
-;;! load = { base = "vmctx", offset = 8 }
-;;!
-;;! [[heaps]]
-;;! base = "heap_base"
-;;! min_size = 0
-;;! offset_guard_size = 0xffffffff
-;;! index_type = "i32"
-;;! style = { kind = "dynamic", bound = "heap_bound" }
-
-(module
-  (memory (export "memory") 0)
-  (func (export "load-without-offset") (param i32) (result i32 i32)
-    local.get 0
-    i32.load
-    local.get 0
-    i32.load
-  )
-  (func (export "load-with-offset") (param i32) (result i32 i32)
-    local.get 0
-    i32.load offset=1234
-    local.get 0
-    i32.load offset=1234
-  )
-)
-
-;; function u0:0(i32, i64 vmctx) -> i32, i32 fast {
-;;     gv0 = vmctx
-;;     gv1 = load.i64 notrap aligned gv0+8
-;;     gv2 = load.i64 notrap aligned gv0
-;;
-;;                                 block0(v0: i32, v1: i64):
-;; @0057                               v4 = uextend.i64 v0
-;; @0057                               v5 = iconst.i64 4
-;; @0057                               v6 = uadd_overflow_trap v4, v5, heap_oob  ; v5 = 4
-;; @0057                               v7 = load.i64 notrap aligned v1+8
-;; @0057                               v8 = load.i64 notrap aligned v1
-;; @0057                               v11 = icmp ugt v6, v7
-;; @0057                               v10 = iconst.i64 0
-;; @0057                               v9 = iadd v8, v4
-;; @0057                               v12 = select_spectre_guard v11, v10, v9  ; v10 = 0
-;; @0057                               v13 = load.i32 little heap v12
-;;                                     v2 -> v13
-;; @005f                               jump block1
-;;
-;;                                 block1:
-;; @005f                               return v13, v13
-;; }
-;;
-;; function u0:1(i32, i64 vmctx) -> i32, i32 fast {
-;;     gv0 = vmctx
-;;     gv1 = load.i64 notrap aligned gv0+8
-;;     gv2 = load.i64 notrap aligned gv0
-;;
-;;                                 block0(v0: i32, v1: i64):
-;; @0064                               v4 = uextend.i64 v0
-;; @0064                               v5 = iconst.i64 1238
-;; @0064                               v6 = uadd_overflow_trap v4, v5, heap_oob  ; v5 = 1238
-;; @0064                               v7 = load.i64 notrap aligned v1+8
-;; @0064                               v8 = load.i64 notrap aligned v1
-;; @0064                               v12 = icmp ugt v6, v7
-;; @0064                               v11 = iconst.i64 0
-;; @0064                               v9 = iadd v8, v4
-;;                                     v26 = iconst.i64 1234
-;; @0064                               v10 = iadd v9, v26  ; v26 = 1234
-;; @0064                               v13 = select_spectre_guard v12, v11, v10  ; v11 = 0
-;; @0064                               v14 = load.i32 little heap v13
-;;                                     v2 -> v14
-;; @006e                               jump block1
-;;
-;;                                 block1:
-;; @006e                               return v14, v14
-;; }