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

[Derek Schuff via Phabricator via llvm-commits]

dschuff accepted this revision.
dschuff added inline comments.
This revision is now accepted and ready to land.


================
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:
> > > 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.
> I think that’s what this CL does..?
Yes; my statement was mostly just a reply to @arsenm to try to clarify what we are doing.

This CL looks fine to me; in any case it doesn't change what we are doing already. 
WRT the comments, I think @arsenm's point is that if we want to prevent optimizations (as the comment in the code seems to indicate) then this isn't the way to do it because it also prevents verification. But I don't think that is actually what we are doing; in any case AFAIK there aren't any optimization passes that run after CFGStackify that try to reason about branches or the CFG, and we are controlling the pass order explicitly in `WebAssemblyPassConfig`.  So maybe we should just clarify the comment to say something like "WebAssembly has control flow that doesn't have explicit branches or direct fallthrough (e.g. try/catch), which cant' be modeled by analyzeBranch. It is created after CFGStackify"


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59740