[Analysis][ValueTracking] Unify most of the tracking between AssumptionCache and DomConditionCache #83161
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-transforms Author: None (goldsteinn) Changes
Full diff: https://github.com/llvm/llvm-project/pull/83161.diff 7 Files Affected:
diff --git a/llvm/include/llvm/Analysis/ConditionCacheUtil.h b/llvm/include/llvm/Analysis/ConditionCacheUtil.h
new file mode 100644
index 00000000000000..9078ac921eaca6
--- /dev/null
+++ b/llvm/include/llvm/Analysis/ConditionCacheUtil.h
@@ -0,0 +1,118 @@
+//===- llvm/Analysis/ConditionCacheUtil.h -----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Shared by DomConditionCache and AssumptionCache. Holds common operation of
+// finding values potentially affected by an assumed/branched on condition.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_ANALYSIS_CONDITIONCACHEUTIL_H
+#define LLVM_ANALYSIS_CONDITIONCACHEUTIL_H
+
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/IR/PatternMatch.h"
+#include <functional>
+
+namespace llvm {
+
+static void addValueAffectedByCondition(
+ Value *V, std::function<void(Value *, int)> InsertAffected, int Idx = -1) {
+ using namespace llvm::PatternMatch;
+ assert(V != nullptr);
+ if (isa<Argument>(V) || isa<GlobalValue>(V)) {
+ InsertAffected(V, Idx);
+ } else if (auto *I = dyn_cast<Instruction>(V)) {
+ InsertAffected(V, Idx);
+
+ // Peek through unary operators to find the source of the condition.
+ Value *Op;
+ if (match(I, m_PtrToInt(m_Value(Op)))) {
+ if (isa<Instruction>(Op) || isa<Argument>(Op))
+ InsertAffected(Op, Idx);
+ }
+ }
+}
+
+static void findValuesAffectedByCondition(
+ Value *Cond, bool IsAssume,
+ std::function<void(Value *, int)> InsertAffected) {
+ using namespace llvm::PatternMatch;
+ auto AddAffected = [&InsertAffected](Value *V) {
+ addValueAffectedByCondition(V, InsertAffected);
+ };
+
+ SmallVector<Value *, 8> Worklist;
+ SmallPtrSet<Value *, 8> Visited;
+ Worklist.push_back(Cond);
+ while (!Worklist.empty()) {
+ Value *V = Worklist.pop_back_val();
+ if (!Visited.insert(V).second)
+ continue;
+
+ CmpInst::Predicate Pred;
+ Value *A, *B, *X;
+
+ if (IsAssume) {
+ AddAffected(V);
+ if (match(V, m_Not(m_Value(X))))
+ AddAffected(X);
+ }
+
+ if (match(V, m_LogicalOp(m_Value(A), m_Value(B)))) {
+ Worklist.push_back(A);
+ Worklist.push_back(B);
+ } else if (match(V, m_Cmp(Pred, m_Value(A), m_Value(B)))) {
+ AddAffected(A);
+ if (IsAssume)
+ AddAffected(B);
+
+ if (ICmpInst::isEquality(Pred)) {
+ if (match(B, m_ConstantInt())) {
+ // (X & C) or (X | C) or (X ^ C).
+ // (X << C) or (X >>_s C) or (X >>_u C).
+ if (match(A, m_BitwiseLogic(m_Value(X), m_ConstantInt())) ||
+ match(A, m_Shift(m_Value(X), m_ConstantInt())))
+ AddAffected(X);
+ }
+ } else {
+ // Handle (A + C1) u< C2, which is the canonical form of
+ // A > C3 && A < C4.
+ if (match(A, m_Add(m_Value(X), m_ConstantInt())) &&
+ match(B, m_ConstantInt()))
+ AddAffected(X);
+
+ // Handle icmp slt/sgt (bitcast X to int), 0/-1, which is supported
+ // by computeKnownFPClass().
+ if (match(A, m_ElementWiseBitCast(m_Value(X)))) {
+ if (Pred == ICmpInst::ICMP_SLT && match(B, m_Zero()))
+ InsertAffected(X, -1);
+ else if (Pred == ICmpInst::ICMP_SGT && match(B, m_AllOnes()))
+ InsertAffected(X, -1);
+ }
+
+ if (CmpInst::isFPPredicate(Pred)) {
+ // fcmp fneg(x), y
+ // fcmp fabs(x), y
+ // fcmp fneg(fabs(x)), y
+ if (match(A, m_FNeg(m_Value(A))))
+ AddAffected(A);
+ if (match(A, m_FAbs(m_Value(A))))
+ AddAffected(A);
+ }
+ }
+ } else if (match(Cond, m_Intrinsic<Intrinsic::is_fpclass>(m_Value(A),
+ m_Value()))) {
+ // Handle patterns that computeKnownFPClass() support.
+ AddAffected(A);
+ }
+ }
+}
+
+} // namespace llvm
+
+#endif
diff --git a/llvm/lib/Analysis/AssumptionCache.cpp b/llvm/lib/Analysis/AssumptionCache.cpp
index 1b7277df0e0cd0..4c0e0db7f9d19e 100644
--- a/llvm/lib/Analysis/AssumptionCache.cpp
+++ b/llvm/lib/Analysis/AssumptionCache.cpp
@@ -16,6 +16,7 @@
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Analysis/AssumeBundleQueries.h"
+#include "llvm/Analysis/ConditionCacheUtil.h"
#include "llvm/Analysis/TargetTransformInfo.h"
#include "llvm/Analysis/ValueTracking.h"
#include "llvm/IR/BasicBlock.h"
@@ -61,20 +62,8 @@ findAffectedValues(CallBase *CI, TargetTransformInfo *TTI,
// Note: This code must be kept in-sync with the code in
// computeKnownBitsFromAssume in ValueTracking.
- auto AddAffected = [&Affected](Value *V, unsigned Idx =
- AssumptionCache::ExprResultIdx) {
- if (isa<Argument>(V) || isa<GlobalValue>(V)) {
- Affected.push_back({V, Idx});
- } else if (auto *I = dyn_cast<Instruction>(V)) {
- Affected.push_back({I, Idx});
-
- // Peek through unary operators to find the source of the condition.
- Value *Op;
- if (match(I, m_PtrToInt(m_Value(Op)))) {
- if (isa<Instruction>(Op) || isa<Argument>(Op))
- Affected.push_back({Op, Idx});
- }
- }
+ auto InsertAffected = [&Affected](Value *V, int Idx) {
+ Affected.push_back({V, Idx < 0 ? AssumptionCache::ExprResultIdx : Idx});
};
for (unsigned Idx = 0; Idx != CI->getNumOperandBundles(); Idx++) {
@@ -82,64 +71,26 @@ findAffectedValues(CallBase *CI, TargetTransformInfo *TTI,
if (Bundle.getTagName() == "separate_storage") {
assert(Bundle.Inputs.size() == 2 &&
"separate_storage must have two args");
- AddAffected(getUnderlyingObject(Bundle.Inputs[0]), Idx);
- AddAffected(getUnderlyingObject(Bundle.Inputs[1]), Idx);
+ addValueAffectedByCondition(getUnderlyingObject(Bundle.Inputs[0]),
+ InsertAffected, Idx);
+ addValueAffectedByCondition(getUnderlyingObject(Bundle.Inputs[1]),
+ InsertAffected, Idx);
} else if (Bundle.Inputs.size() > ABA_WasOn &&
Bundle.getTagName() != IgnoreBundleTag)
- AddAffected(Bundle.Inputs[ABA_WasOn], Idx);
+ addValueAffectedByCondition(Bundle.Inputs[ABA_WasOn], InsertAffected,
+ Idx);
}
- Value *Cond = CI->getArgOperand(0), *A, *B;
- AddAffected(Cond);
- if (match(Cond, m_Not(m_Value(A))))
- AddAffected(A);
-
- CmpInst::Predicate Pred;
- if (match(Cond, m_Cmp(Pred, m_Value(A), m_Value(B)))) {
- AddAffected(A);
- AddAffected(B);
-
- if (Pred == ICmpInst::ICMP_EQ) {
- if (match(B, m_ConstantInt())) {
- Value *X;
- // (X & C) or (X | C) or (X ^ C).
- // (X << C) or (X >>_s C) or (X >>_u C).
- if (match(A, m_BitwiseLogic(m_Value(X), m_ConstantInt())) ||
- match(A, m_Shift(m_Value(X), m_ConstantInt())))
- AddAffected(X);
- }
- } else if (Pred == ICmpInst::ICMP_NE) {
- Value *X;
- // Handle (X & pow2 != 0).
- if (match(A, m_And(m_Value(X), m_Power2())) && match(B, m_Zero()))
- AddAffected(X);
- } else if (Pred == ICmpInst::ICMP_ULT) {
- Value *X;
- // Handle (A + C1) u< C2, which is the canonical form of A > C3 && A < C4,
- // and recognized by LVI at least.
- if (match(A, m_Add(m_Value(X), m_ConstantInt())) &&
- match(B, m_ConstantInt()))
- AddAffected(X);
- } else if (CmpInst::isFPPredicate(Pred)) {
- // fcmp fneg(x), y
- // fcmp fabs(x), y
- // fcmp fneg(fabs(x)), y
- if (match(A, m_FNeg(m_Value(A))))
- AddAffected(A);
- if (match(A, m_FAbs(m_Value(A))))
- AddAffected(A);
- }
- } else if (match(Cond, m_Intrinsic<Intrinsic::is_fpclass>(m_Value(A),
- m_Value(B)))) {
- AddAffected(A);
- }
+ Value *Cond = CI->getArgOperand(0);
+ findValuesAffectedByCondition(Cond, /*IsAssume*/ true, InsertAffected);
if (TTI) {
const Value *Ptr;
unsigned AS;
std::tie(Ptr, AS) = TTI->getPredicatedAddrSpace(Cond);
if (Ptr)
- AddAffected(const_cast<Value *>(Ptr->stripInBoundsOffsets()));
+ addValueAffectedByCondition(
+ const_cast<Value *>(Ptr->stripInBoundsOffsets()), InsertAffected);
}
}
diff --git a/llvm/lib/Analysis/DomConditionCache.cpp b/llvm/lib/Analysis/DomConditionCache.cpp
index da05e02b4b57f7..fadd0a6f22953b 100644
--- a/llvm/lib/Analysis/DomConditionCache.cpp
+++ b/llvm/lib/Analysis/DomConditionCache.cpp
@@ -7,75 +7,14 @@
//===----------------------------------------------------------------------===//
#include "llvm/Analysis/DomConditionCache.h"
-#include "llvm/IR/PatternMatch.h"
+#include "llvm/Analysis/ConditionCacheUtil.h"
using namespace llvm;
-using namespace llvm::PatternMatch;
-// TODO: This code is very similar to findAffectedValues() in
-// AssumptionCache, but currently specialized to just the patterns that
-// computeKnownBits() supports, and without the notion of result elem indices
-// that are AC specific. Deduplicate this code once we have a clearer picture
-// of how much they can be shared.
static void findAffectedValues(Value *Cond,
SmallVectorImpl<Value *> &Affected) {
- auto AddAffected = [&Affected](Value *V) {
- if (isa<Argument>(V) || isa<GlobalValue>(V)) {
- Affected.push_back(V);
- } else if (auto *I = dyn_cast<Instruction>(V)) {
- Affected.push_back(I);
-
- // Peek through unary operators to find the source of the condition.
- Value *Op;
- if (match(I, m_PtrToInt(m_Value(Op)))) {
- if (isa<Instruction>(Op) || isa<Argument>(Op))
- Affected.push_back(Op);
- }
- }
- };
-
- SmallVector<Value *, 8> Worklist;
- SmallPtrSet<Value *, 8> Visited;
- Worklist.push_back(Cond);
- while (!Worklist.empty()) {
- Value *V = Worklist.pop_back_val();
- if (!Visited.insert(V).second)
- continue;
-
- CmpInst::Predicate Pred;
- Value *A, *B;
- if (match(V, m_LogicalOp(m_Value(A), m_Value(B)))) {
- Worklist.push_back(A);
- Worklist.push_back(B);
- } else if (match(V, m_ICmp(Pred, m_Value(A), m_Constant()))) {
- AddAffected(A);
-
- if (ICmpInst::isEquality(Pred)) {
- Value *X;
- // (X & C) or (X | C) or (X ^ C).
- // (X << C) or (X >>_s C) or (X >>_u C).
- if (match(A, m_BitwiseLogic(m_Value(X), m_ConstantInt())) ||
- match(A, m_Shift(m_Value(X), m_ConstantInt())))
- AddAffected(X);
- } else {
- Value *X;
- // Handle (A + C1) u< C2, which is the canonical form of
- // A > C3 && A < C4.
- if (match(A, m_Add(m_Value(X), m_ConstantInt())))
- AddAffected(X);
- // Handle icmp slt/sgt (bitcast X to int), 0/-1, which is supported by
- // computeKnownFPClass().
- if ((Pred == ICmpInst::ICMP_SLT || Pred == ICmpInst::ICMP_SGT) &&
- match(A, m_ElementWiseBitCast(m_Value(X))))
- Affected.push_back(X);
- }
- } else if (match(Cond, m_CombineOr(m_FCmp(Pred, m_Value(A), m_Constant()),
- m_Intrinsic<Intrinsic::is_fpclass>(
- m_Value(A), m_Constant())))) {
- // Handle patterns that computeKnownFPClass() support.
- AddAffected(A);
- }
- }
+ auto InsertAffected = [&Affected](Value *V, int) { Affected.push_back(V); };
+ findValuesAffectedByCondition(Cond, /*IsAssume*/ false, InsertAffected);
}
void DomConditionCache::registerBranch(BranchInst *BI) {
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index e591ac504e9f05..39d358b45d9021 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -806,15 +806,13 @@ void llvm::computeKnownBitsFromContext(const Value *V, KnownBits &Known,
if (Depth == MaxAnalysisRecursionDepth)
continue;
- ICmpInst *Cmp = dyn_cast<ICmpInst>(Arg);
- if (!Cmp)
+ if (!isa<ICmpInst>(Arg) && !match(Arg, m_LogicalOp(m_Value(), m_Value())))
continue;
if (!isValidAssumeForContext(I, Q.CxtI, Q.DT))
continue;
- computeKnownBitsFromCmp(V, Cmp->getPredicate(), Cmp->getOperand(0),
- Cmp->getOperand(1), Known, Q);
+ computeKnownBitsFromCond(V, Arg, Known, /*Depth*/ 0, Q, /*Invert*/ false);
}
// Conflicting assumption: Undefined behavior will occur on this execution
diff --git a/llvm/test/Analysis/ValueTracking/numsignbits-from-assume.ll b/llvm/test/Analysis/ValueTracking/numsignbits-from-assume.ll
index 00c66eeb599572..95ac98532da621 100644
--- a/llvm/test/Analysis/ValueTracking/numsignbits-from-assume.ll
+++ b/llvm/test/Analysis/ValueTracking/numsignbits-from-assume.ll
@@ -51,7 +51,7 @@ define i32 @computeNumSignBits_sub1(i32 %in) {
define i32 @computeNumSignBits_sub2(i32 %in) {
; CHECK-LABEL: @computeNumSignBits_sub2(
-; CHECK-NEXT: [[SUB:%.*]] = add i32 [[IN:%.*]], -1
+; CHECK-NEXT: [[SUB:%.*]] = add nsw i32 [[IN:%.*]], -1
; CHECK-NEXT: [[COND:%.*]] = icmp ult i32 [[SUB]], 43
; CHECK-NEXT: call void @llvm.assume(i1 [[COND]])
; CHECK-NEXT: [[SH:%.*]] = shl nuw nsw i32 [[SUB]], 3
diff --git a/llvm/test/Transforms/InstCombine/fpclass-from-dom-cond.ll b/llvm/test/Transforms/InstCombine/fpclass-from-dom-cond.ll
index d40cd7fd503ecc..d6706d76056eea 100644
--- a/llvm/test/Transforms/InstCombine/fpclass-from-dom-cond.ll
+++ b/llvm/test/Transforms/InstCombine/fpclass-from-dom-cond.ll
@@ -185,10 +185,9 @@ define i1 @test8(float %x) {
; CHECK-NEXT: [[COND:%.*]] = fcmp oeq float [[ABS]], 0x7FF0000000000000
; CHECK-NEXT: br i1 [[COND]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]]
; CHECK: if.then:
-; CHECK-NEXT: [[RET1:%.*]] = call i1 @llvm.is.fpclass.f32(float [[X]], i32 575)
-; CHECK-NEXT: ret i1 [[RET1]]
+; CHECK-NEXT: ret i1 true
; CHECK: if.else:
-; CHECK-NEXT: [[RET2:%.*]] = call i1 @llvm.is.fpclass.f32(float [[X]], i32 575)
+; CHECK-NEXT: [[RET2:%.*]] = call i1 @llvm.is.fpclass.f32(float [[X]], i32 59)
; CHECK-NEXT: ret i1 [[RET2]]
;
%abs = call float @llvm.fabs.f32(float %x)
diff --git a/llvm/test/Transforms/InstSimplify/assume_icmp.ll b/llvm/test/Transforms/InstSimplify/assume_icmp.ll
index 9ac3f468ab604f..2ebb3b23a9b883 100644
--- a/llvm/test/Transforms/InstSimplify/assume_icmp.ll
+++ b/llvm/test/Transforms/InstSimplify/assume_icmp.ll
@@ -93,14 +93,10 @@ define void @and(i32 %x, i32 %y, i32 %z) {
; CHECK-NEXT: [[CMP2:%.*]] = icmp ugt i32 [[Z:%.*]], [[Y]]
; CHECK-NEXT: [[AND:%.*]] = and i1 [[CMP1]], [[CMP2]]
; CHECK-NEXT: call void @llvm.assume(i1 [[AND]])
-; CHECK-NEXT: [[CMP3:%.*]] = icmp ugt i32 [[X]], [[Y]]
-; CHECK-NEXT: call void @use(i1 [[CMP3]])
-; CHECK-NEXT: [[CMP4:%.*]] = icmp uge i32 [[X]], [[Y]]
-; CHECK-NEXT: call void @use(i1 [[CMP4]])
-; CHECK-NEXT: [[CMP5:%.*]] = icmp ugt i32 [[Z]], [[Y]]
-; CHECK-NEXT: call void @use(i1 [[CMP5]])
-; CHECK-NEXT: [[CMP6:%.*]] = icmp uge i32 [[Z]], [[Y]]
-; CHECK-NEXT: call void @use(i1 [[CMP6]])
+; CHECK-NEXT: call void @use(i1 true)
+; CHECK-NEXT: call void @use(i1 true)
+; CHECK-NEXT: call void @use(i1 true)
+; CHECK-NEXT: call void @use(i1 true)
; CHECK-NEXT: ret void
;
%cmp1 = icmp ugt i32 %x, %y
|
goldsteinn
changed the title
goldsteinn
changed the title
dtcxzyw
reviewed
Feb 27, 2024
goldsteinn
force-pushed
the
perf/goldsteinn/unified-tracking-cache-final
branch
from
d5541ce
to
23dac25
Compare
dtcxzyw
added a commit
to dtcxzyw/llvm-opt-benchmark
that referenced
this pull request
Feb 27, 2024
goldsteinn
force-pushed
the
perf/goldsteinn/unified-tracking-cache-final
branch
from
23dac25
to
e6a8771
Compare
|
@dtcxzyw just re-pushed. Its mostly no change (realized the second patch was NFC for DomConditionCache) but final patch is basically the same. Edit: only functional change is fixed what seems like incorrect value matching |
nikic
requested changes
Feb 27, 2024
why no |
llvm-project/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp Lines 2828 to 2847 in 9ca8db3 |
Dropped adding dom condition w.o constant RHS. Edit: I see @dtcxzyw's comment. Dropping. |
goldsteinn
force-pushed
the
perf/goldsteinn/unified-tracking-cache-final
branch
from
e6a8771
to
c32b70e
Compare
What about |
goldsteinn
force-pushed
the
perf/goldsteinn/unified-tracking-cache-final
branch
from
c32b70e
to
1578c39
Compare
Dropped assume(logicop). Left comment/potential todo. |
dtcxzyw
requested changes
Feb 29, 2024
llvm/lib/Analysis/ValueTracking.cpp
Outdated
| @@ -813,6 +813,8 @@ void llvm::computeKnownBitsFromContext(const Value *V, KnownBits &Known, | |||
| if (!isValidAssumeForContext(I, Q.CxtI, Q.DT)) | |||
| continue; | |||
|
|
|||
| // Change to `computeKnownBitsFromCond` if we start supporting assume of | |||
| // logic op in ConditionCacheUtil. | |||
There was a problem hiding this comment.
Conditions for @llvm.assume should be atomic expressions. TBH I don't think we can benefit from handling logical ops here.
There was a problem hiding this comment.
Something like assume(a || b) can exist.
I'm happy to drop supporting it in this patch, but would prefer to keep comments indicating the steps necessary should we change out mind.
that okay?
There was a problem hiding this comment.
Something like
assume(a || b)can exist.
Yeah, but it would not benefit the optimization if we fail to merge a || b.
There was a problem hiding this comment.
What do you mean we fail to merge the a || b?
There was a problem hiding this comment.
Due to recent unfortunate events, I would recommend against leaving any TODO/FIXME comments unless you are very sure that resolving them is actually valuable. Otherwise there is too much risk that new contributors will try to "fix" them without having the necessary background.
|
NB: @nikic and @dtcxzyw, after the changes there is no longer any notable compile time impact due to this patch: |
goldsteinn
force-pushed
the
perf/goldsteinn/unified-tracking-cache-final
branch
from
1578c39
to
2379c6c
Compare
dtcxzyw
approved these changes
Mar 1, 2024
nikic
reviewed
Mar 1, 2024
llvm/lib/Analysis/ValueTracking.cpp
Outdated
| @@ -813,6 +813,8 @@ void llvm::computeKnownBitsFromContext(const Value *V, KnownBits &Known, | |||
| if (!isValidAssumeForContext(I, Q.CxtI, Q.DT)) | |||
| continue; | |||
|
|
|||
| // Change to `computeKnownBitsFromCond` if we start supporting assume of | |||
| // logic op in ConditionCacheUtil. | |||
There was a problem hiding this comment.
Due to recent unfortunate events, I would recommend against leaving any TODO/FIXME comments unless you are very sure that resolving them is actually valuable. Otherwise there is too much risk that new contributors will try to "fix" them without having the necessary background.
nikic
reviewed
Mar 1, 2024
Dropped TODOs |
goldsteinn
force-pushed
the
perf/goldsteinn/unified-tracking-cache-final
branch
2 times, most recently
from
d95e976
to
ceb30f7
Compare
nikic
reviewed
Mar 4, 2024
goldsteinn
force-pushed
the
perf/goldsteinn/unified-tracking-cache-final
branch
from
ceb30f7
to
9b1f1ec
Compare
dtcxzyw
reviewed
Mar 4, 2024
Comment on lines
1199
to
1200
| // Call `InsertAffected` on all Values whose known bits / value may be affected | ||
| // the condition `Cond`. Used by AssumptionCache and DomConditionCache. |
There was a problem hiding this comment.
Suggested change
| // Call `InsertAffected` on all Values whose known bits / value may be affected | |
| // the condition `Cond`. Used by AssumptionCache and DomConditionCache. | |
| /// Call \p InsertAffected on all Values whose known bits / value may be affected by | |
| /// the condition \p Cond. Used by AssumptionCache and DomConditionCache. |
There was a problem hiding this comment.
https://llvm.org/docs/CodingStandards.html#comment-formatting
In general, prefer C++-style comments (// for normal comments, /// for doxygen documentation comments).
| AddAffected(A); | ||
| } | ||
| Value *Cond = CI->getArgOperand(0); | ||
| llvm::findValuesAffectedByCondition(Cond, /*IsAssume*/ true, InsertAffected); |
There was a problem hiding this comment.
Suggested change
| llvm::findValuesAffectedByCondition(Cond, /*IsAssume*/ true, InsertAffected); | |
| llvm::findValuesAffectedByCondition(Cond, /*IsAssume=*/ true, InsertAffected); |
There was a problem hiding this comment.
Huh, is this our preferred style? I always used what @goldsteinn wrote here.
There was a problem hiding this comment.
https://llvm.org/docs/CodingStandards.html#comment-formatting
An in-line C-style comment makes the intent obvious:
Object.emitName(/*Prefix=*/nullptr);
There was a problem hiding this comment.
Clang-format accepts two styles, either /*IsAssume*/ true or /*IsAssume=*/true (the space difference is important -- /*IsAssume=*/ true is not valid.) My question here is why we would want to use the latter over the former.
nikic
reviewed
Mar 4, 2024
…ssumptionCache; NFC
…ConditionCache This helps cover some missing cases in both and hopefully serves as creating an easier framework for extending general condition based analysis.
goldsteinn
force-pushed
the
perf/goldsteinn/unified-tracking-cache-final
branch
from
9b1f1ec
to
fd3e521
Compare
nikic
approved these changes
Mar 4, 2024
| @@ -1195,6 +1195,12 @@ std::optional<bool> isImpliedByDomCondition(CmpInst::Predicate Pred, | |||
| const Value *LHS, const Value *RHS, | |||
| const Instruction *ContextI, | |||
| const DataLayout &DL); | |||
|
|
|||
| // Call \p InsertAffected on all Values whose known bits / value may be affected | |||
| // by the condition \p Cond. Used by AssumptionCache and DomConditionCache. | |||
There was a problem hiding this comment.
Still uses // instead of ///.
There was a problem hiding this comment.
bah, ill update before I push.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DomConditionCache::findAffectedValuesto a new file; NFCfindAffectedValuesbetween DomConditionCache and AssumptionCache; NFCThere is no significant compile time impact due to this patch:
https://llvm-compile-time-tracker.com/compare.php?from=f410f74cd5b26319b5796e0404c6a0f3b5cc00a5&to=1578c397fffa4f90755c3f2458d1c381228ef631&stat=instructions%3Au