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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1297
+      // available.
+      if (DepInfo.isUnknown()) {
+        UnavailableBlocks.push_back(DepBB);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131776