From: Heejin Ahn Date: Tue, 26 Mar 2019 18:21:20 +0000 (+0000) Subject: [WebAssembly] Don't analyze branches after CFGStackify X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=0d02e66ea8ce85300e686ccba5ae4e0806a875e2;p=llvm [WebAssembly] Don't analyze branches after CFGStackify Summary: `WebAssembly::analyzeBranch` now does not analyze anything if the function is CFG stackified. We were previously doing similar things by checking if a branch's operand is whether an integer or an MBB, but this failed to bail out when a BB did not have any terminators. Consider this case: ``` bb0: try $label0 call @foo // unwinds to %ehpad bb1: ... br $label0 // jumps to %cont. can be deleted ehpad: catch ... cont: end_try ``` Here `br $label0` will be deleted in CFGStackify's `removeUnnecessaryInstrs` function, because we jump to the %cont block even without the branch. But in this case, MachineVerifier fails to verify this, because `ehpad` is not a successor of `bb1` even if `bb1` does not have any terminators. MachineVerifier incorrectly thinks `bb1` falls through to the next block. This pass now consistently rejects all analysis after CFGStackify whether a BB has terminators or not, also making the MachineVerifier work. (MachineVerifier does not try to verify relationships between BBs if `analyzeBranch` fails, the behavior we want after CFGStackify.) This also adds a new option `-wasm-disable-ehpad-sort` for testing. This option helps create the sorted order we want to test, and without the fix in this patch, the tests in cfg-stackify-eh.ll fail at MachineVerifier with `-wasm-disable-ehpad-sort`. Reviewers: dschuff Subscribers: sunfish, sbc100, jgravelle-google, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D59740 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@357015 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Target/WebAssembly/WebAssemblyCFGSort.cpp b/lib/Target/WebAssembly/WebAssemblyCFGSort.cpp index 98bdfafef52..4c5d0192fc2 100644 --- a/lib/Target/WebAssembly/WebAssemblyCFGSort.cpp +++ b/lib/Target/WebAssembly/WebAssemblyCFGSort.cpp @@ -34,6 +34,14 @@ using namespace llvm; #define DEBUG_TYPE "wasm-cfg-sort" +// Option to disable EH pad first sorting. Only for testing unwind destination +// mismatches in CFGStackify. +static cl::opt WasmDisableEHPadSort( + "wasm-disable-ehpad-sort", cl::ReallyHidden, + cl::desc( + "WebAssembly: Disable EH pad-first sort order. Testing purpose only."), + cl::init(false)); + namespace { // Wrapper for loops and exceptions @@ -187,10 +195,12 @@ namespace { struct CompareBlockNumbers { bool operator()(const MachineBasicBlock *A, const MachineBasicBlock *B) const { - if (A->isEHPad() && !B->isEHPad()) - return false; - if (!A->isEHPad() && B->isEHPad()) - return true; + if (!WasmDisableEHPadSort) { + if (A->isEHPad() && !B->isEHPad()) + return false; + if (!A->isEHPad() && B->isEHPad()) + return true; + } return A->getNumber() > B->getNumber(); } @@ -199,11 +209,12 @@ struct CompareBlockNumbers { struct CompareBlockNumbersBackwards { bool operator()(const MachineBasicBlock *A, const MachineBasicBlock *B) const { - // We give a higher priority to an EH pad - if (A->isEHPad() && !B->isEHPad()) - return false; - if (!A->isEHPad() && B->isEHPad()) - return true; + if (!WasmDisableEHPadSort) { + if (A->isEHPad() && !B->isEHPad()) + return false; + if (!A->isEHPad() && B->isEHPad()) + return true; + } return A->getNumber() < B->getNumber(); } diff --git a/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp b/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp index a7c120963e0..3f5aba087f0 100644 --- a/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp +++ b/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp @@ -101,6 +101,13 @@ bool WebAssemblyInstrInfo::analyzeBranch(MachineBasicBlock &MBB, MachineBasicBlock *&FBB, SmallVectorImpl &Cond, bool /*AllowModify*/) const { + const auto &MFI = *MBB.getParent()->getInfo(); + // WebAssembly has control flow that doesn't have explicit branches or direct + // fallthrough (e.g. try/catch), which can't be modeled by analyzeBranch. It + // is created after CFGStackify. + if (MFI.isCFGStackified()) + return true; + bool HaveCond = false; for (MachineInstr &MI : MBB.terminators()) { switch (MI.getOpcode()) { @@ -110,9 +117,6 @@ bool WebAssemblyInstrInfo::analyzeBranch(MachineBasicBlock &MBB, case WebAssembly::BR_IF: if (HaveCond) return true; - // If we're running after CFGStackify, we can't optimize further. - if (!MI.getOperand(0).isMBB()) - return true; Cond.push_back(MachineOperand::CreateImm(true)); Cond.push_back(MI.getOperand(1)); TBB = MI.getOperand(0).getMBB(); @@ -121,18 +125,12 @@ bool WebAssemblyInstrInfo::analyzeBranch(MachineBasicBlock &MBB, case WebAssembly::BR_UNLESS: if (HaveCond) return true; - // If we're running after CFGStackify, we can't optimize further. - if (!MI.getOperand(0).isMBB()) - return true; Cond.push_back(MachineOperand::CreateImm(false)); Cond.push_back(MI.getOperand(1)); TBB = MI.getOperand(0).getMBB(); HaveCond = true; break; case WebAssembly::BR: - // If we're running after CFGStackify, we can't optimize further. - if (!MI.getOperand(0).isMBB()) - return true; if (!HaveCond) TBB = MI.getOperand(0).getMBB(); else @@ -141,9 +139,6 @@ bool WebAssemblyInstrInfo::analyzeBranch(MachineBasicBlock &MBB, case WebAssembly::BR_ON_EXN: if (HaveCond) return true; - // If we're running after CFGStackify, we can't optimize further. - if (!MI.getOperand(0).isMBB()) - return true; Cond.push_back(MachineOperand::CreateImm(true)); Cond.push_back(MI.getOperand(2)); TBB = MI.getOperand(0).getMBB(); diff --git a/test/CodeGen/WebAssembly/cfg-stackify-eh.ll b/test/CodeGen/WebAssembly/cfg-stackify-eh.ll index a93babedd13..f11c4ff2367 100644 --- a/test/CodeGen/WebAssembly/cfg-stackify-eh.ll +++ b/test/CodeGen/WebAssembly/cfg-stackify-eh.ll @@ -1,5 +1,6 @@ ; RUN: llc < %s -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -disable-block-placement -verify-machineinstrs -fast-isel=false -machine-sink-split-probability-threshold=0 -cgp-freq-ratio-to-skip-merge=1000 -exception-model=wasm -mattr=+exception-handling | FileCheck %s ; RUN: llc < %s -O0 -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -verify-machineinstrs -exception-model=wasm -mattr=+exception-handling | FileCheck %s --check-prefix=NOOPT +; RUN: llc < %s -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -disable-block-placement -verify-machineinstrs -fast-isel=false -machine-sink-split-probability-threshold=0 -cgp-freq-ratio-to-skip-merge=1000 -exception-model=wasm -mattr=+exception-handling -wasm-disable-ehpad-sort target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128" target triple = "wasm32-unknown-unknown"