[PATCH] D149668: [ShrinkWrap] Use underlying object to rule out stack access.

[Jonathon Penix via Phabricator via llvm-commits]

jpenix-quic added inline comments.


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:286
+  if (MI.mayLoadOrStore() && (MI.isCall() || MI.hasUnmodeledSideEffects() ||
+                              !all_of(MI.memoperands(), IsKnownNonStackPtr)))
     return true;
----------------
fhahn wrote:
> jpenix-quic wrote:
> > Hi @fhahn, does this work as expected when MI.memoperands() is empty? If I understand correctly, all_of will return true in that case which I don't think is what we want (my understanding is we need to be conservative if memoperands() is empty). That said, I struggled to find a test where this causes an issue here, so I'm not sure if this is actually an issue in practice or if any possible situations are covered by the other checks, etc.
> > 
> > Do you have thoughts on this? If it is an issue, I imagine we could just add an extra check for the empty case or something similar.
> @jpenix-quic I had a closer look and added a case where missing memoperands cause incorrect results. It's a bit artificial as memory operands are dropped from instructions that usually have them, but it illustrates the problem: ac9e9594af37
> 
> I also pushed a fix to treat instructions without memoperands conservatively: d0718ff410cc
> 
> 
Great, thank you for looking into and handling this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149668