[PATCH] D59740: [WebAssembly] Don't analyze branches after CFGStackify

Mon Mar 25 17:22:59 PDT 2019

dschuff added inline comments.

================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp:106-107
+  // If we're running after CFGStackify, we can't optimize further.
+  if (MFI.isCFGStackified())
+    return true;
+
----------------
aheejin wrote:
> dschuff wrote:
> > aheejin wrote:
> > > arsenm wrote:
> > > > I think this isn't what you want to do. analyzeBranch isn't really for turning off transformations. The verifier will stop catching useful cases with this
> > > Please note that we were doing this already. (The part of the code I deleted)
> > > CFGStackify is a pass near the end of WebAssembly compilation pipeline, and this pass deletes all MBB operands from branches to replace them with integers, and also deletes some other unnecessary instruction from WebAssembly standpoint, so after that it is not possible to analyze branch anyway. This patch is not introducing a new thing; it is rather filling in a hole, because we rejected analysis only when there were terminators and ignored the case of fall-throughs after CFGStackify.
> > I guess if we actually modeled the wasm control flow structure outside of the CFGStackify pass (e.g. the BeginToEnd mappings etc), then analyzeBranch could use them and it could return the correct successors. But since we don't yet, the control flow really is not analyzable by analyzeBranch and it should return false, right? And as a result any optimization or verification that depends on that can't run.
> Not sure what you mean, but this function unintuitively returns false when it is analyzable, and true if not. Please refer to the [[ https://github.com/llvm/llvm-project/blob/0200d62ec7a54b79313bc2c4ccbe51c2a8c5e940/llvm/include/llvm/CodeGen/TargetInstrInfo.h#L546-L578 | description ]] of its return values.
sorry yes, I meant that it should return true. I'm just saying I think that this behavior is probably consistent with the intention of analyzeBranch. Once it can no longer correctly determine the destination, analyzeBranch should fail.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59740/new/

https://reviews.llvm.org/D59740