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

Fri May 12 08:17:00 PDT 2023

fhahn marked an inline comment as done.
fhahn 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;
----------------
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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149668