[PATCH] D131776: [GVN] Fix miscompile shown in github issue #57025

Fri Sep 2 03:56:00 PDT 2022

bjope abandoned this revision.
bjope added a comment.

Abandoned. Problem was solved in D131776 <https://reviews.llvm.org/D131776>.

================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1297
+      // available.
+      if (DepInfo.isUnknown()) {
+        UnavailableBlocks.push_back(DepBB);
----------------
nikic wrote:
> bjope wrote:
> > I've tried replacing this with `assert(!DepInfo.isUnknown())` and it seems like that is a totally untested scenario (except for the new test/Transforms/GVN/memdep-unknown-deadblocks.ll test case).
> > 
> > So even if D131776 is a better fix in some sense, I'm still wondering what would be correct to do if getting here with a DepInfo that is `Unknown`. At least when I looked at this a couple of weeks ago I got the feeling that the behavior without this patch could be wrong and that adding to UnavailableBlocks would be more correct as we do not know if there is a dependent mem-op or not. Or am I missing something.
> > 
> > Given that D131776 probably will hide this code path also for the memdep-unknown-deadblocks.ll test case, then I'm not sure how to motivate this fix. If we never expect to get here with DepInfo.isUnknown, then maybe we should assert on that instead.
> > 
> > I'll try to run some fuzzy testing to see if I can trigger this code path even with the fix from D131776.
> I don't think this code path cares about the value of DepInfo at all. If the block is unreachable, then we can always assume it produces a poison value.
Right. Well, I have been able to trigger this code path with a `DepInfo.isUnknown()` here, but haven't found any case where the change in this patch makes a difference to the end result (given that I have merged the fix from D131776).

So I'm not doing any further investigation of this right now, and will just abondon this issue.

Btw, thanks a lot for the help (and with the more correct fix in D131776). Very much appreciated!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131776