Skip to content
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

[NFC][Load] Make ScanFrom required parameters #100789

Merged

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Jul 26, 2024

In #100773 we will go conservative for sanitizers,
so it's better to pinpoint location consciously.

For #100639.

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 26, 2024

@llvm/pr-subscribers-llvm-analysis

Author: Vitaly Buka (vitalybuka)

Changes

In #100773 we will go conservative for sanitizers,
so it's better to pinpoint location consciously.


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

3 Files Affected:

  • (modified) llvm/include/llvm/Analysis/Loads.h (+2-2)
  • (modified) polly/lib/Analysis/ScopBuilder.cpp (+1-1)
  • (modified) polly/lib/Analysis/ScopDetection.cpp (+2-1)
diff --git a/llvm/include/llvm/Analysis/Loads.h b/llvm/include/llvm/Analysis/Loads.h
index 33e817828b754..03476ef4e14ec 100644
--- a/llvm/include/llvm/Analysis/Loads.h
+++ b/llvm/include/llvm/Analysis/Loads.h
@@ -70,7 +70,7 @@ bool isDereferenceableAndAlignedPointer(const Value *V, Align Alignment,
 /// the address is already accessed.
 bool isSafeToLoadUnconditionally(Value *V, Align Alignment, const APInt &Size,
                                  const DataLayout &DL,
-                                 Instruction *ScanFrom = nullptr,
+                                 Instruction *ScanFrom,
                                  AssumptionCache *AC = nullptr,
                                  const DominatorTree *DT = nullptr,
                                  const TargetLibraryInfo *TLI = nullptr);
@@ -101,7 +101,7 @@ bool isDereferenceableReadOnlyLoop(Loop *L, ScalarEvolution *SE,
 /// the address is already accessed.
 bool isSafeToLoadUnconditionally(Value *V, Type *Ty, Align Alignment,
                                  const DataLayout &DL,
-                                 Instruction *ScanFrom = nullptr,
+                                 Instruction *ScanFrom,
                                  AssumptionCache *AC = nullptr,
                                  const DominatorTree *DT = nullptr,
                                  const TargetLibraryInfo *TLI = nullptr);
diff --git a/polly/lib/Analysis/ScopBuilder.cpp b/polly/lib/Analysis/ScopBuilder.cpp
index d594823410f5f..0b9a1a916e1c1 100644
--- a/polly/lib/Analysis/ScopBuilder.cpp
+++ b/polly/lib/Analysis/ScopBuilder.cpp
@@ -2770,7 +2770,7 @@ isl::set ScopBuilder::getNonHoistableCtx(MemoryAccess *Access,
 
   auto &DL = scop->getFunction().getDataLayout();
   if (isSafeToLoadUnconditionally(LI->getPointerOperand(), LI->getType(),
-                                  LI->getAlign(), DL)) {
+                                  LI->getAlign(), DL, nullptr)) {
     SafeToLoad = isl::set::universe(AccessRelation.get_space().range());
   } else if (BB != LI->getParent()) {
     // Skip accesses in non-affine subregions as they might not be executed
diff --git a/polly/lib/Analysis/ScopDetection.cpp b/polly/lib/Analysis/ScopDetection.cpp
index eab7bd83e6a4e..79db3965de023 100644
--- a/polly/lib/Analysis/ScopDetection.cpp
+++ b/polly/lib/Analysis/ScopDetection.cpp
@@ -490,7 +490,8 @@ bool ScopDetection::onlyValidRequiredInvariantLoads(
 
     for (auto NonAffineRegion : Context.NonAffineSubRegionSet) {
       if (isSafeToLoadUnconditionally(Load->getPointerOperand(),
-                                      Load->getType(), Load->getAlign(), DL))
+                                      Load->getType(), Load->getAlign(), DL,
+                                      nullptr))
         continue;
 
       if (NonAffineRegion->contains(Load) &&

Copy link

github-actions bot commented Jul 26, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@vitalybuka vitalybuka changed the base branch from main to users/vitalybuka/spr/nfcload-find-better-place-for-mustsuppressspeculation July 26, 2024 18:55
vitalybuka added a commit that referenced this pull request Jul 26, 2024
In #100773 we will go conservative for sanitizers,
so it's better to pinpoint location consciously.

Pull Request: #100789
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/nfcload-make-scanfrom-required-parameters branch from d61be92 to 403561b Compare July 26, 2024 18:59
@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
In #100773 we will go conservative for sanitizers,
so it's better to pinpoint location consciously.

Pull Request: #100789
@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/nfcload-make-scanfrom-required-parameters branch from 403561b to 0fce1ba Compare July 27, 2024 00:53
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

Base automatically changed from users/vitalybuka/spr/nfcload-find-better-place-for-mustsuppressspeculation to main July 29, 2024 17:29
Created using spr 1.3.4
@vitalybuka vitalybuka merged commit 07f3a08 into main Jul 29, 2024
4 of 6 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/nfcload-make-scanfrom-required-parameters branch July 29, 2024 17:30
banach-space pushed a commit to banach-space/llvm-project that referenced this pull request Aug 7, 2024
In llvm#100773 we will go conservative for sanitizers,
so it's better to pinpoint location consciously.

For llvm#100639.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants