Skip to content

[NFC][Load] Find better place for mustSuppressSpeculation #100794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Jul 26, 2024

And extract suppressSpeculativeLoadForSanitizers.

For #100639.

@vitalybuka vitalybuka requested a review from nikic as a code owner July 26, 2024 18:19
@llvmbot llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Jul 26, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 26, 2024

@llvm/pr-subscribers-llvm-analysis

Author: Vitaly Buka (vitalybuka)

Changes

And extract suppressSpeculativeLoadForSanitizers.


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

4 Files Affected:

  • (modified) llvm/include/llvm/Analysis/Loads.h (+7)
  • (modified) llvm/include/llvm/Analysis/ValueTracking.h (-7)
  • (modified) llvm/lib/Analysis/Loads.cpp (+15)
  • (modified) llvm/lib/Analysis/ValueTracking.cpp (-11)
diff --git a/llvm/include/llvm/Analysis/Loads.h b/llvm/include/llvm/Analysis/Loads.h
index 33e817828b754..38f86f77b4158 100644
--- a/llvm/include/llvm/Analysis/Loads.h
+++ b/llvm/include/llvm/Analysis/Loads.h
@@ -106,6 +106,13 @@ bool isSafeToLoadUnconditionally(Value *V, Type *Ty, Align Alignment,
                                  const DominatorTree *DT = nullptr,
                                  const TargetLibraryInfo *TLI = nullptr);
 
+/// Return true if speculation of the given load must be suppressed to avoid
+/// ordering or interfering with an active sanitizer.  If not suppressed,
+/// dereferenceability and alignment must be proven separately.  Note: This
+/// is only needed for raw reasoning; if you use the interface below
+/// (isSafeToSpeculativelyExecute), this is handled internally.
+bool mustSuppressSpeculation(const LoadInst &LI);
+
 /// The default number of maximum instructions to scan in the block, used by
 /// FindAvailableLoadedValue().
 extern cl::opt<unsigned> DefMaxInstsToScan;
diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index 5ef6e43483906..96fa16970584d 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -792,13 +792,6 @@ bool onlyUsedByLifetimeMarkers(const Value *V);
 /// droppable instructions.
 bool onlyUsedByLifetimeMarkersOrDroppableInsts(const Value *V);
 
-/// Return true if speculation of the given load must be suppressed to avoid
-/// ordering or interfering with an active sanitizer.  If not suppressed,
-/// dereferenceability and alignment must be proven separately.  Note: This
-/// is only needed for raw reasoning; if you use the interface below
-/// (isSafeToSpeculativelyExecute), this is handled internally.
-bool mustSuppressSpeculation(const LoadInst &LI);
-
 /// Return true if the instruction does not have any effects besides
 /// calculating the result and does not have undefined behavior.
 ///
diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
index 61c6aa5e5a3eb..e2ab425bbdc03 100644
--- a/llvm/lib/Analysis/Loads.cpp
+++ b/llvm/lib/Analysis/Loads.cpp
@@ -345,6 +345,21 @@ bool llvm::isDereferenceableAndAlignedInLoop(LoadInst *LI, Loop *L,
                                             HeaderFirstNonPHI, AC, &DT);
 }
 
+static bool suppressSpeculativeLoadForSanitizers(const Function &F) {
+  // Speculative load may create a race that did not exist in the source.
+  return F.hasFnAttribute(Attribute::SanitizeThread) ||
+         // Speculative load may load data from dirty regions.
+         F.hasFnAttribute(Attribute::SanitizeAddress) ||
+         F.hasFnAttribute(Attribute::SanitizeHWAddress);
+}
+
+bool llvm::mustSuppressSpeculation(const LoadInst &LI) {
+  if (!LI.isUnordered())
+    return true;
+  const Function &F = *LI.getFunction();
+  return suppressSpeculativeLoadForSanitizers(F);
+}
+
 /// Check if executing a load of this pointer value cannot trap.
 ///
 /// If DT and ScanFrom are specified this method performs context-sensitive
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index bfd26fadd237b..497f6eafd22d8 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -6798,17 +6798,6 @@ bool llvm::onlyUsedByLifetimeMarkersOrDroppableInsts(const Value *V) {
       V, /* AllowLifetime */ true, /* AllowDroppable */ true);
 }
 
-bool llvm::mustSuppressSpeculation(const LoadInst &LI) {
-  if (!LI.isUnordered())
-    return true;
-  const Function &F = *LI.getFunction();
-  // Speculative load may create a race that did not exist in the source.
-  return F.hasFnAttribute(Attribute::SanitizeThread) ||
-    // Speculative load may load data from dirty regions.
-    F.hasFnAttribute(Attribute::SanitizeAddress) ||
-    F.hasFnAttribute(Attribute::SanitizeHWAddress);
-}
-
 bool llvm::isSafeToSpeculativelyExecute(const Instruction *Inst,
                                         const Instruction *CtxI,
                                         AssumptionCache *AC,

vitalybuka added a commit that referenced this pull request Jul 26, 2024
And extract `suppressSpeculativeLoadForSanitizers`.

Pull Request: #100794
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/nfcload-find-better-place-for-mustsuppressspeculation branch from 9572ea0 to ffe82da Compare July 26, 2024 19:01
Some optimization need to be undone with
sanitizers by #100773.

Pull Request: #100844
And extract `suppressSpeculativeLoadForSanitizers`.

Pull Request: #100794
@vitalybuka vitalybuka changed the base branch from main to users/vitalybuka/spr/nfcinstcombinesroaasan-precommit-test-affected-by-100773 July 27, 2024 00:52
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/nfcload-find-better-place-for-mustsuppressspeculation branch from ffe82da to a33fda5 Compare July 27, 2024 00:53
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/nfcinstcombinesroaasan-precommit-test-affected-by-100773 branch from dd4531e to 3eb7a8e Compare July 27, 2024 00:59
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vitalybuka vitalybuka changed the base branch from users/vitalybuka/spr/nfcinstcombinesroaasan-precommit-test-affected-by-100773 to main July 29, 2024 17:27
Created using spr 1.3.4
@vitalybuka vitalybuka merged commit 945dd9a into main Jul 29, 2024
4 of 6 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/nfcload-find-better-place-for-mustsuppressspeculation branch July 29, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:analysis Includes value tracking, cost tables and constant folding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants