[DSE] Consider the aliasing through global variable while checking clobber by haopliu · Pull Request #120044 · llvm/llvm-project

haopliu · 2024-12-16T06:30:16Z

While update the read clobber check for the "initializes" attr, we checked the aliasing among arguments, but didn't consider the aliasing through global variable. It causes problems in this example:

int g_var = 123;

void update(int* ptr) {
  *ptr = g_var;

void foo() {
  g_var = 0;
  bar(&g_var);
}

We mistakenly removed g_var = 0; as a dead store.

Fix the issue by requiring the CallBase only access argmem or inaccessiblemem.

…bber

llvmbot · 2024-12-16T06:30:48Z

@llvm/pr-subscribers-llvm-transforms

Author: Haopeng Liu (haopliu)

Changes

While update the read clobber check for the "initializes" attr, we checked the aliasing among arguments, but didn't consider the aliasing through global variable. It causes problems in this example:

int g_var = 123;

void update(int* ptr) {
  *ptr = g_var;

void foo() {
  g_var = 0;
  bar(g_var);
}

We mistakenly removed g_var = 0; as a dead store.

Fix the issue by requiring the CallBase only access argmem or inaccessiblemem.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp (+8-3)
(modified) llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll (+34)

diff --git a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
index 4799640089fa9a..19f6bba583941b 100644
--- a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -1665,11 +1665,16 @@ struct DSEState {
       // original MD. Stop walk.
       // If KillingDef is a CallInst with "initializes" attribute, the reads in
       // the callee would be dominated by initializations, so it should be safe.
+      // Note that in `getInitializesArgMemLoc`, we only check the aliasing
+      // among arguments. If aliasing through global variables, we consider it
+      // as read clobber.
       bool IsKillingDefFromInitAttr = false;
       if (IsInitializesAttrMemLoc) {
-        if (KillingI == UseInst &&
-            KillingUndObj == getUnderlyingObject(MaybeDeadLoc.Ptr))
-          IsKillingDefFromInitAttr = true;
+        if (auto *CB = dyn_cast<CallBase>(UseInst))
+          IsKillingDefFromInitAttr =
+              KillingI == UseInst &&
+              KillingUndObj == getUnderlyingObject(MaybeDeadLoc.Ptr) &&
+              CB->onlyAccessesInaccessibleMemOrArgMem();
       }
 
       if (isReadClobber(MaybeDeadLoc, UseInst) && !IsKillingDefFromInitAttr) {
diff --git a/llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll b/llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll
index d93da9b6612b05..9c040d4d5bb380 100644
--- a/llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll
+++ b/llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll
@@ -2,12 +2,22 @@
 ; RUN: opt < %s -passes=dse -enable-dse-initializes-attr-improvement -S | FileCheck %s
 
 declare void @p1_write_only(ptr nocapture noundef writeonly initializes((0, 2)) dead_on_unwind)
+
 declare void @p1_write_then_read(ptr nocapture noundef initializes((0, 2)) dead_on_unwind)
+  memory(argmem: readwrite, inaccessiblemem: readwrite)
+
 declare void @p1_clobber(ptr nocapture noundef)
+
 declare void @p2_same_range(ptr nocapture noundef initializes((0, 2)) dead_on_unwind, ptr nocapture noundef initializes((0, 2)) dead_on_unwind)
+  memory(argmem: readwrite, inaccessiblemem: readwrite)
+
 declare void @p2_no_init(ptr nocapture noundef initializes((0, 2)) dead_on_unwind, ptr nocapture noundef dead_on_unwind)
+
 declare void @p2_no_dead_on_unwind(ptr nocapture noundef initializes((0, 2)) dead_on_unwind, ptr nocapture noundef initializes((0, 2)))
+  memory(argmem: readwrite, inaccessiblemem: readwrite)
+
 declare void @p2_no_dead_on_unwind_but_nounwind(ptr nocapture noundef initializes((0, 2)) dead_on_unwind, ptr nocapture noundef initializes((0, 2))) nounwind
+  memory(argmem: readwrite, inaccessiblemem: readwrite)
 
 ; Function Attrs: mustprogress nounwind uwtable
 define i16 @p1_write_only_caller() {
@@ -215,8 +225,12 @@ define i16 @p2_no_dead_on_unwind_but_nounwind_alias_caller() {
 }
 
 declare void @llvm.memset.p0.i64(ptr nocapture, i8, i64, i1) nounwind
+
 declare void @large_p1(ptr nocapture noundef initializes((0, 200))) nounwind
+  memory(argmem: readwrite, inaccessiblemem: readwrite)
+
 declare void @large_p2(ptr nocapture noundef initializes((0, 200)), ptr nocapture noundef initializes((0, 100))) nounwind
+  memory(argmem: readwrite, inaccessiblemem: readwrite)
 
 ; Function Attrs: mustprogress nounwind uwtable
 define i16 @large_p1_caller() {
@@ -299,3 +313,23 @@ define i16 @large_p2_may_or_partial_alias_caller2(ptr %base1, ptr %base2) {
   ret i16 %l
 }
 
+@g = global i16 123, align 2
+
+declare void @read_global(ptr nocapture noundef initializes((0, 2)))
+  memory(read, argmem: write, inaccessiblemem: none) nounwind
+
+define i16 @global_var_alias() {
+; CHECK-LABEL: @global_var_alias(
+; CHECK-NEXT:    store i32 0, ptr @g, align 4
+; CHECK-NEXT:    [[G_ADDR:%.*]] = getelementptr i32, ptr @g, i64 1
+; CHECK-NEXT:    call void @read_global(ptr [[G_ADDR]])
+; CHECK-NEXT:    [[L:%.*]] = load i16, ptr @g, align 2
+; CHECK-NEXT:    ret i16 [[L]]
+;
+  store i32 0, ptr @g, align 4
+  %g_addr = getelementptr i32, ptr @g, i64 1
+  call void @read_global(ptr %g_addr)
+  %l = load i16, ptr @g
+  ret i16 %l
+}
+

nikic · 2024-12-16T09:15:41Z

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

+          IsKillingDefFromInitAttr =
+              KillingI == UseInst &&
+              KillingUndObj == getUnderlyingObject(MaybeDeadLoc.Ptr) &&
+              CB->onlyAccessesInaccessibleMemOrArgMem();


Would it make more sense to handle this inside getInitializesArgMemLoc?

Moved this check to inside getInitializesArgMemLoc. Thanks!

aeubanks

I think it's unfortunate to require onlyAccessesInaccessibleMemOrArgMem() for all of the cases, it'll probably prevent a lot of valid DSE. can we be more precise on when we need onlyAccessesInaccessibleMemOrArgMem()? if the pointer hasn't escaped yet then we don't need this restriction

aeubanks · 2024-12-16T21:28:50Z

llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll

+; CHECK-NEXT:    [[L:%.*]] = load i16, ptr @g, align 2
+; CHECK-NEXT:    ret i16 [[L]]
+;
+  store i32 0, ptr @g, align 4


isn't this storing an i32 value to an i16 global? and the gep is out of bounds?

Ah, fixed the mistake. Thanks.

haopliu · 2024-12-21T20:31:06Z

I think it's unfortunate to require onlyAccessesInaccessibleMemOrArgMem() for all of the cases, it'll probably prevent a lot of valid DSE. can we be more precise on when we need onlyAccessesInaccessibleMemOrArgMem()? if the pointer hasn't escaped yet then we don't need this restriction

Updated the check to require either the argument is from Alloca that doesn't escape or CB->onlyAccessesInaccessibleMemOrArgMem. Otherwise, discard the "initializes" info.

PTAL, thanks.

nikic · 2024-12-21T21:40:03Z

The check for alloca should be a check for isIdentifiedFunctionLocal and your ValidFromAlloca function should be PointerMayBeCaptured -- though it probably makes sense to query it via EarliestEscapeAnalysis, which is a bit more precise and cached.

haopliu · 2024-12-22T06:59:55Z

Thanks! Updated the check to use isIdentifiedFunctionLocal and EarliestEscapeAnalysis::isNotCapturedBefore.

nikic · 2024-12-22T08:49:54Z

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

+// Return true if "Arg" is function local and isn't captured before "CB" or
+// if "Arg" is GEP whose base pointer is function local and isn't captured
+// before "CB".
+bool IsFuncLocalAndNotCaptured(Value *Arg, const CallBase *CB,


Suggested change

bool IsFuncLocalAndNotCaptured(Value *Arg, const CallBase *CB,

bool isFuncLocalAndNotCaptured(Value *Arg, const CallBase *CB,

nikic · 2024-12-22T08:50:05Z

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

+                               EarliestEscapeAnalysis &EA) {
+  if (isIdentifiedFunctionLocal(Arg))
+    return EA.isNotCapturedBefore(Arg, CB, /*OrAt*/ true);
+  const auto *GEP = dyn_cast<GetElementPtrInst>(Arg);


Check the result of getUnderlyingObject instead, it will look through multiple GEPs.

Ah, done. Thanks!

jvoung · 2024-12-22T18:52:45Z

llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll

+;
+  store i16 0, ptr @g, align 4
+  %g_addr = getelementptr i8, ptr @g, i64 1
+  call void @read_global(ptr %g_addr)


it seems like this case would still go out of bounds for the i16 global?
%g_addr = 1 byte from the start
then @read_global initializes 2 bytes from there?

Ooops, removed the GEP and directly read/write @g. Thanks.

jvoung

LGTM w.r.t. my comments

Nice recommendations for isIdentifiedFunctionLocal( and isNotCapturedBefore =)

nikic

LGTM

nikic · 2025-01-09T15:06:34Z

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

+    // either it's function local and isn't captured before or the "CB" only
+    // accesses arg or inaccessible mem.
+    if (!Inits.empty() && !isFuncLocalAndNotCaptured(CurArg, CB, EA) &&
+        !CB->onlyAccessesInaccessibleMemOrArgMem())


I'd check onlyAccessesInaccessibleMemOrArgMem before isFuncLocalAndNotCaptured as it's cheaper.

Ah, thanks for the reminder! Done.

nikic · 2025-01-09T15:06:47Z

llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll

 }

+declare void @fn_capture(ptr noundef)
+; Function Attrs: mustprogress nounwind uwtable


Drop inaccurate Function Attrs comment.

nikic · 2025-01-09T15:06:58Z

llvm/test/Transforms/DeadStoreElimination/inter-procedural.ll

  ret i16 %l
 }

+declare void @fn_capture(ptr noundef)


Drop noundef, it's not relevant here.

(Retry) enable the initializes improvement in DSE. Initially enabled in #119116. Fix the aliasing issue through global variables in #120044. The compile-time comparison of this enabling (no meaningful diff): https://llvm-compile-time-tracker.com/compare.php?from=b46fcb9fa32f24660b1b8858d5c4cbdb76ef9d8b&to=33dc817b81f7898c87b052d1ddfd3d6e6f5b5dbd&stat=instructions%3Au

(Retry) enable the initializes improvement in DSE. Initially enabled in llvm/llvm-project#119116. Fix the aliasing issue through global variables in llvm/llvm-project#120044. The compile-time comparison of this enabling (no meaningful diff): https://llvm-compile-time-tracker.com/compare.php?from=b46fcb9fa32f24660b1b8858d5c4cbdb76ef9d8b&to=33dc817b81f7898c87b052d1ddfd3d6e6f5b5dbd&stat=instructions%3Au

Consider the aliasing through global variable while checking read clo…

69d43a5

…bber

haopliu requested review from aeubanks, jvoung and nikic December 16, 2024 06:30

llvmbot added the llvm:transforms label Dec 16, 2024

dtcxzyw changed the title ~~Consider the aliasing through global variable while checking clobber~~ [DSE] Consider the aliasing through global variable while checking clobber Dec 16, 2024

nikic reviewed Dec 16, 2024

View reviewed changes

aeubanks reviewed Dec 16, 2024

View reviewed changes

haopliu added 2 commits December 21, 2024 20:21

Check whether an arg can from Alloca that doesn't escape

fa652d9

Fix a unit test mistake

defcdb1

Add an unit test about alloca then escape

1ff8ebc

Use isIdentifiedFunctionLocal and isNotCapturedBefore to check arg

56d75ec

nikic reviewed Dec 22, 2024

View reviewed changes

Use getUnderlyingObject to check GEP or pointer cast

64fe614

jvoung reviewed Dec 22, 2024

View reviewed changes

Fix a unit test mistake

6626f36

jvoung approved these changes Jan 9, 2025

View reviewed changes

nikic approved these changes Jan 9, 2025

View reviewed changes

flip the cond check order and update unit test

5dc849e

haopliu merged commit fed817a into llvm:main Jan 14, 2025
4 checks passed

This was referenced Jan 23, 2025

[DSE] Enable the initializes improvement in DSE #124058

Merged

[clang] Miscompilation at -Os #119646

Closed

	bool IsFuncLocalAndNotCaptured(Value Arg, const CallBase CB,
	bool isFuncLocalAndNotCaptured(Value Arg, const CallBase CB,

Conversation

haopliu commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeubanks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haopliu commented Dec 21, 2024

Uh oh!

nikic commented Dec 21, 2024

Uh oh!

haopliu commented Dec 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvoung left a comment

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

haopliu commented Dec 16, 2024 •

edited

Loading