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

Thu Sep 1 06:07:49 PDT 2022

nikic added inline comments.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131776