[PATCH] D125515: [WebAssembly] Fix register use-def in FixIrreducibleControlFlow

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp:108
+  // set.
+  MF.getProperties().set(MachineFunctionProperties::Property::TracksLiveness);
+
----------------
dschuff wrote:
> aheejin wrote:
> > dschuff wrote:
> > > aheejin wrote:
> > > > dschuff wrote:
> > > > > aheejin wrote:
> > > > > > arsenm wrote:
> > > > > > > aheejin wrote:
> > > > > > > > dschuff wrote:
> > > > > > > > > Should this go in the FixIrreducibleControlFlow pass? It's that pass that sets up the implicit defs correctly and not this one, right?
> > > > > > > > It can go in any pass before OptimizeLiveIntervals pass. We can put that in FixIrreducibleCFG, but I'm not sure if that pass is any special, because every pass preserves correct register def-use relationship (except for FixIrreducibleControlFlow, until now) and `IMPLICIT_DEF` is just a way of preserving it in that pass. Also, note that while we only add `IMPLICIT_DEF`s in case of irreducible CFGs, we have to set this for every function, whether or not it is irreducible.
> > > > > > > This should move into the pass's getSetProperties/getClearedProperties
> > > > > > Thanks for the info! I wasn't aware of these APIs. But come to think about it, I don't think we even need that, because I deleted `invalidateLiveness` call in this pass above. I'm not sure why it was there in the first place, because we don't do anything to invalidate liveness here; what this pass does is to replace physical registers with virtual ones 1-to-1, which has nothing to do with liveness.
> > > > > > 
> > > > > > Also I removed `invalidateLiveness` call in FixIrreducibleControlFlow pass; we now add `IMPLICIT_DEF`s there so it should be fine.
> > > > > > 
> > > > > > So in short we don't invalidate liveness up until CFGSort/CFGStackify, which is later than this and near the end of the pipeline, and the thing we invalidate there is `kill` flag and not use-def relationship. So I don't think we need to set this anywhere. 
> > > > > > 
> > > > > > But to make things clear, I'll instead add this to WebAssemblyOptimizeLiveIntervals, the next pass, to make sure what this pass requires.
> > > > > > ```
> > > > > > MachineFunctionProperties getRequiredProperties() const override {
> > > > > >   return MachineFunctionProperties().set(
> > > > > >       MachineFunctionProperties::Property::TracksLiveness);
> > > > > > }
> > > > > > ```
> > > > > so you're saying, none of these passes now either set or clear the property, they just preserve it; so there's no need to mark it explicitly (other than ReplacePhysRegs where we actually remove the physregs). I think that makes sense.
> > > > Oh, sorry, that's not what I meant. I made the confusion because I also added `getSetProperties`.. That was an incorrect snippet I added while experimenting.
> > > > 
> > > > > So you're saying, none of these passes now either set or clear the property, they just preserve it;
> > > > This is correct.
> > > > 
> > > > > so there's no need to mark it explicitly (other than ReplacePhysRegs where we actually remove the physregs).
> > > > I believe even ReplacePhysRegs doesn't need to remove this property. Note that I deleted the call to `invalidateLiveness` on line 77 in this file. So we don't need to set the property again here, meaning we don't need go call `getSetProperties`. This was added in the diff, which made the confusion. I deleted the call.
> > > > I believe even ReplacePhysRegs doesn't need to remove this property. Note that I deleted the call to invalidateLiveness on line 77 in this file. So we don't need to set the property again here, meaning we don't need go call getSetProperties. This was added in the diff, which made the confusion. I deleted the call.
> > > 
> > > Sorry I actually was seeing that in the last patch set it *added* the property. I assumed that was because it was only the physregs that may have been violating it. But yes it still makes sense that none of the passes needs to set or clear the property, and mark OptimizeLiveIntervals as requiring it.
> > Yeah that was confusing. So these register liveins only apply to physical registers, so after we remove all physical registers here, we don't violate livein requirements for sure. But were we violating the requirements before, even if we were using physical registers before this pass? Those physical registers are `sp` and `fp`, and they are added in PrologEpilogInserter or somewhere, and I think that pass should take care of preserving that. And a brief 1 min look on that pass looks like it does. Even if it doesn't, it's surely not that we should remove the property here; we should have rather set it here. (But as I said, we didn't remove it anywhere, so we don't need to set it again)
> yeah, makes sense now. thanks for checking `sp` and `fp`. This patchset LGTM.
sp and fp are presumably reserved registers, and thus not added to liveins lists or have tracked liveness anyway


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125515