[SDAG] Introduce inbounds flag for pointer arithmetic #131862
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ritter-x2a
requested review from
antoniofrighetto,
arsenm,
dtcxzyw,
nikic,
s-barannikov and
shiltian
ritter-x2a
added
the
llvm:SelectionDAG
Graphite App
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-llvm-selectiondag Author: Fabian Ritter (ritter-x2a) ChangesThis patch introduces an inbounds SDNodeFlag, to show that a pointer Inbounds information is useful in the ISel when selecting memory A similar patch for gMIR and GlobalISel will follow. For SWDEV-516125. Full diff: https://github.com/llvm/llvm-project/pull/131862.diff 4 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
index 2283f99202e2f..13ac65f5d731c 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
@@ -415,12 +415,15 @@ struct SDNodeFlags {
Unpredictable = 1 << 13,
// Compare instructions which may carry the samesign flag.
SameSign = 1 << 14,
+ // Pointer arithmetic instructions that remain in bounds, e.g., implementing
+ // an inbounds GEP.
+ InBounds = 1 << 15,
// NOTE: Please update LargestValue in LLVM_DECLARE_ENUM_AS_BITMASK below
// the class definition when adding new flags.
PoisonGeneratingFlags = NoUnsignedWrap | NoSignedWrap | Exact | Disjoint |
- NonNeg | NoNaNs | NoInfs | SameSign,
+ NonNeg | NoNaNs | NoInfs | SameSign | InBounds,
FastMathFlags = NoNaNs | NoInfs | NoSignedZeros | AllowReciprocal |
AllowContract | ApproximateFuncs | AllowReassociation,
};
@@ -455,6 +458,7 @@ struct SDNodeFlags {
void setAllowReassociation(bool b) { setFlag<AllowReassociation>(b); }
void setNoFPExcept(bool b) { setFlag<NoFPExcept>(b); }
void setUnpredictable(bool b) { setFlag<Unpredictable>(b); }
+ void setInBounds(bool b) { setFlag<InBounds>(b); }
// These are accessors for each flag.
bool hasNoUnsignedWrap() const { return Flags & NoUnsignedWrap; }
@@ -472,6 +476,7 @@ struct SDNodeFlags {
bool hasAllowReassociation() const { return Flags & AllowReassociation; }
bool hasNoFPExcept() const { return Flags & NoFPExcept; }
bool hasUnpredictable() const { return Flags & Unpredictable; }
+ bool hasInBounds() const { return Flags & InBounds; }
bool operator==(const SDNodeFlags &Other) const {
return Flags == Other.Flags;
@@ -481,7 +486,7 @@ struct SDNodeFlags {
};
LLVM_DECLARE_ENUM_AS_BITMASK(decltype(SDNodeFlags::None),
- SDNodeFlags::SameSign);
+ SDNodeFlags::InBounds);
inline SDNodeFlags operator|(SDNodeFlags LHS, SDNodeFlags RHS) {
LHS |= RHS;
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 14bb1d943d2d6..f2e5cb64d5b52 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -4293,6 +4293,7 @@ void SelectionDAGBuilder::visitGetElementPtr(const User &I) {
if (NW.hasNoUnsignedWrap() ||
(int64_t(Offset) >= 0 && NW.hasNoUnsignedSignedWrap()))
Flags |= SDNodeFlags::NoUnsignedWrap;
+ Flags.setInBounds(NW.isInBounds());
N = DAG.getNode(ISD::ADD, dl, N.getValueType(), N,
DAG.getConstant(Offset, dl, N.getValueType()), Flags);
@@ -4336,6 +4337,7 @@ void SelectionDAGBuilder::visitGetElementPtr(const User &I) {
if (NW.hasNoUnsignedWrap() ||
(Offs.isNonNegative() && NW.hasNoUnsignedSignedWrap()))
Flags.setNoUnsignedWrap(true);
+ Flags.setInBounds(NW.isInBounds());
OffsVal = DAG.getSExtOrTrunc(OffsVal, dl, N.getValueType());
@@ -4398,6 +4400,7 @@ void SelectionDAGBuilder::visitGetElementPtr(const User &I) {
// pointer index type (add nuw).
SDNodeFlags AddFlags;
AddFlags.setNoUnsignedWrap(NW.hasNoUnsignedWrap());
+ AddFlags.setInBounds(NW.isInBounds());
N = DAG.getNode(ISD::ADD, dl, N.getValueType(), N, IdxN, AddFlags);
}
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
index 64ecff8d71f98..6360deb1ed528 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
@@ -663,6 +663,9 @@ void SDNode::print_details(raw_ostream &OS, const SelectionDAG *G) const {
if (getFlags().hasSameSign())
OS << " samesign";
+ if (getFlags().hasInBounds())
+ OS << " inbounds";
+
if (getFlags().hasNonNeg())
OS << " nneg";
diff --git a/llvm/test/CodeGen/X86/merge-store-partially-alias-loads.ll b/llvm/test/CodeGen/X86/merge-store-partially-alias-loads.ll
index c1fdd71c04948..6e16f946f8ac0 100644
--- a/llvm/test/CodeGen/X86/merge-store-partially-alias-loads.ll
+++ b/llvm/test/CodeGen/X86/merge-store-partially-alias-loads.ll
@@ -16,7 +16,7 @@
; DBGDAG-LABEL: Optimized legalized selection DAG: %bb.0 'merge_store_partial_overlap_load:'
; DBGDAG: [[ENTRYTOKEN:t[0-9]+]]: ch,glue = EntryToken
; DBGDAG-DAG: [[BASEPTR:t[0-9]+]]: i64,ch = CopyFromReg [[ENTRYTOKEN]],
-; DBGDAG-DAG: [[ADDPTR:t[0-9]+]]: i64 = add {{(nuw )?}}[[BASEPTR]], Constant:i64<2>
+; DBGDAG-DAG: [[ADDPTR:t[0-9]+]]: i64 = add {{(nuw )?}}{{(inbounds )?}}[[BASEPTR]], Constant:i64<2>
; DBGDAG-DAG: [[LD2:t[0-9]+]]: i16,ch = load<(load (s16) from %ir.tmp81, align 1)> [[ENTRYTOKEN]], [[BASEPTR]], undef:i64
; DBGDAG-DAG: [[LD1:t[0-9]+]]: i8,ch = load<(load (s8) from %ir.tmp12)> [[ENTRYTOKEN]], [[ADDPTR]], undef:i64
|
|
This seems semantically ambiguous. In GlobalISel, you have G_PTR_ADD, and inbounds on that has an obvious meaning; G_PTR_ADD has basically the same semantics as getelementptr. But in SelectionDAG, we don't have that; we just have a plain ISD::ADD. How do you tell which operand is the pointer? The obvious heuristic is outright wrong: you can't assume a value is being used as a pointer just because its value is equal to the address of some variable. |
I see your point. We could define the semantics of an inbounds ISD::ADD to be "The result is an address into an allocated object and (exactly) one of the operands is an address into the same allocated object." What do you think about a "ConstInBoundsOffset" flag instead (suggestions for better names would be much appreciated), meaning "One operand is a constant offset and the other is an address pointing into the same allocated object as the result"? |
|
Saying "one side is inbounds of the other side" is basically useless, as far as I can tell, for almost any transform. The other possibility you mentioned is that we say one side is a constant, the other is not, and the non-constant side must be a pointer? That seems fragile. The semantics of the add depend on the "syntactic" form of the operands, which could be rewritten. Not really great. Maybe we could consider adding "ISD::PTRADD"? Lowers to ISD::ADD by default, but targets that want to do weird things with pointer arithmetic could do them. One other concern, which applies to basically any formulation of this: Since SelectionDAG doesn't have a distinct pointer type, you can't tell whether the pointer operand was produced by an inttoptr. So in some cases, you have an operation marked "inbounds", but it's ambiguous which object it's actually inbounds to. This isn't really a problem at the moment because we do IR-level transforms that remove inttoptr anyway, but if we ever do resolve the IR-level issues, we should have some idea for how we propagate the fix to SelectionDAG. |
|
There is a ISD::PTRADD in #105669, which still has some open questions about whether the AMDGPU use of the tablegen |
That would be helpful. We'd still need an inbounds flag for ISD::PTRADD, but it would certainly be easier to make use of. I'll look into that.
I can see that's a problem if you'd want to infer that an operation is inbounds, or if you'd want to prove the absence (or presence) of poison/UB. But how is that a problem for generating code? If there is an inbounds flag on a (hypothetical) ISD::PTRADD, we can assume that the operation is inbounds with respect to whatever the address operand is pointing to, no matter if it's the result of integer operations, right? |
Say you have two adjacent objects In some cases, you might be able to disambiguate based on the RHS of the ptradd; if the offset is positive, it must be inbounds relative to |
ritter-x2a
force-pushed
the
users/ritter-x2a/03-17-_sdag_introduce_inbounds_flag_for_pointer_arithmetic
branch
from
4d8dda5 to
efdf7eb
Compare
ritter-x2a
force-pushed
the
users/ritter-x2a/03-10-_separateconstoffsetfromgep_preserve_inbounds_flag_based_on_valuetracking
branch
from
5d1a65b to
7af5dfd
Compare
I should have been more specific there, I meant "with respect to whatever the address operand is based on". As I read the pointer aliasing rules in the langref, the pointee depends on how the address was derived, not on the bit value of the address. Admittedly, it looks like the semantics in the langref falls apart if the address operand is based on both objects, e.g., if the address is a result of inttoptr where pointers to both objects "contribute" to the integer value. |
This patch introduces an inbounds SDNodeFlag, to show that a pointer addition SDNode implements an inbounds getelementptr operation (i.e., the pointer operand is in bounds wrt. the allocated object it is based on, and the arithmetic does not change that). The flag is set in the DAG construction when lowering inbounds GEPs. Inbounds information is useful in the ISel when selecting memory instructions that perform address computations whose intermediate steps must be in the same memory region as the final result. A follow-up patch will start using it for AMDGPU's flat memory instructions, where the immediate offset must not affect the memory aperture of the address. A similar patch for gMIR and GlobalISel will follow. For SWDEV-516125.
ritter-x2a
force-pushed
the
users/ritter-x2a/03-17-_sdag_introduce_inbounds_flag_for_pointer_arithmetic
branch
from
efdf7eb to
bd22087
Compare
|
If an pointer is constructed using inttoptr, it can be based on multiple objects. (In IR, we can see the inttoptr, but in SelectionDAG, it's treated as a noop and eliminated.) The "inbounds" rule should probably say something like this: "The base pointer must be based on one or more allocated objects for which the following applies: the base pointer has an in bounds address of the allocated object, and the resulting pointer is in bounds of the allocated object." Actually, thinking about it a bit more, the "allocated object" referenced in the LangRef spec doesn't actually have to be live. So you also have to worry about objects which were previously allocated at the same address... which means inbounds is basically meaningless for a pointer created using inttoptr. |
This patch introduces an inbounds SDNodeFlag, to show that a pointer
addition SDNode implements an inbounds getelementptr operation (i.e.,
the pointer operand is in bounds wrt. the allocated object it is based
on, and the arithmetic does not change that). The flag is set in the DAG
construction when lowering inbounds GEPs.
Inbounds information is useful in the ISel when selecting memory
instructions that perform address computations whose intermediate steps
must be in the same memory region as the final result. A follow-up patch
will start using it for AMDGPU's flat memory instructions, where the
immediate offset must not affect the memory aperture of the address.
A similar patch for gMIR and GlobalISel will follow.
For SWDEV-516125.