MemoryDependenceAnalysis: Consider a pointer clobbered if it is saved

[wzssyqa]

In MemoryDependenceResults::getSimplePointerDependencyFrom, when we find the instruction that a LoadInst depends, if we find an store instruction that save the pointer used by LoadInst, we consider the pointer may be clobbered.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: YunQiang Su (wzssyqa)

Changes

In MemoryDependenceResults::getSimplePointerDependencyFrom, when we find the instruction that a LoadInst depends, if we find an store instruction that save the pointer used by LoadInst, we consider the pointer may be clobbered.

---

Full diff: https://github.com/llvm/llvm-project/pull/142096.diff

1 Files Affected:

• (modified) llvm/lib/Analysis/MemoryDependenceAnalysis.cpp (+5)

diff --git a/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp b/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
index f062189bac6a0..00f36b9e088ad 100644
--- a/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
@@ -564,6 +564,11 @@ MemDepResult MemoryDependenceResults::getSimplePointerDependencyFrom(
         if (!QueryInst || QueryInst->isVolatile())
           return MemDepResult::getClobber(SI);
 
+      // If we store the pointer of QueryInst, it is danger due to that the address
+      // may be modified with other reference.
+      if (QueryInst && QueryInst->getOperand(0) == SI->getOperand(0))
+        return MemDepResult::getClobber(SI);
+
       // If alias analysis can tell that this store is guaranteed to not modify
       // the query pointer, ignore it.  Use getModRefInfo to handle cases where
       // the query pointer points to constant memory etc.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- llvm/lib/Analysis/MemoryDependenceAnalysis.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp b/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
index 00f36b9e0..a907e22ba 100644
--- a/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
@@ -564,8 +564,8 @@ MemDepResult MemoryDependenceResults::getSimplePointerDependencyFrom(
         if (!QueryInst || QueryInst->isVolatile())
           return MemDepResult::getClobber(SI);
 
-      // If we store the pointer of QueryInst, it is danger due to that the address
-      // may be modified with other reference.
+      // If we store the pointer of QueryInst, it is danger due to that the
+      // address may be modified with other reference.
       if (QueryInst && QueryInst->getOperand(0) == SI->getOperand(0))
         return MemDepResult::getClobber(SI);
 

[wzssyqa]

I find this problem when working on x264.
The attached is the IR file.

./bin/opt --passes="default<O3>" x264.ll -S -o xx.ll

can reproduce this problem.

The difference is in function x264_8_macroblock_cache_allocate:
a load instruction

%82 = load ptr, ptr %slice_table, align 16, !tbaa !545

in baiscblock cond.end380 is remove mistakenly.

In fact the contents of ptr %slice_table is clobbered in while.body.

For C code, please see:
https://github.com/ShiftMediaProject/x264/blob/2597c3645a721f4ab1870f13a3751edf78328ff3/common/macroblock.c#L248

x264.tgz

I failed to reduce this cases, thus I cannot figure out a test case.

[nikic]

This should be handled by AA/CaptureTracking. Needs a reduced reproducer to understand why it fails.

[wzssyqa]

This should be handled by AA/CaptureTracking. Needs a reduced reproducer to understand why it fails.

In the comment of AA/CaptureTracking

// This file contains routines that help determine which pointers are captured.
// A pointer value is captured if the function makes a copy of any part of the
// pointer that outlives the call.  Not being captured means, more or less, that
// the pointer is only dereferenced and not stored in a global.  Returning part
// of the pointer as the function return value may or may not count as capturing
// the pointer, depending on the context.

In our case, the pointer is not globalized. It just store into a local vector, and then we edit the value of local vector.

[nikic]

What "capture" means also depends on context. A store into a local variable is a "capture" for the purposes of AA inside the function, and will be treated as such.

Btw, the first thing to check if you get AA-related miscompiles in C/C++ code is whether it works with -fno-strict-aliasing. The cause is almost always strict aliasing violations. If it works with -fno-strict-aliasing, you can use -fsanitize=type to identify the cause.