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.

Sign up for GitHub

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

Jump to bottom

[IR] Require that ptrmask mask matches pointer index size #69343

Merged
merged 3 commits into from
Oct 24, 2023
Merged

[IR] Require that ptrmask mask matches pointer index size #69343

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cae33d1
[IR] Require that ptrmask mask matches pointer index size
nikic Oct 17, 2023
446be93
Drop leftover sentence, clarify no capturing
nikic Oct 19, 2023
1a61193
Rephrase to be more applicable to non-integral pointers
nikic Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 16 additions & 5 deletions llvm/docs/LangRef.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26952,7 +26952,8 @@ Arguments:
""""""""""

The first argument is a pointer or vector of pointers. The second argument is
an integer or vector of integers.
an integer or vector of integers with the same bit width as the index type
size of the first argument.

Overview:
""""""""""
Expand All @@ -26965,10 +26966,20 @@ to facilitate alias analysis and underlying-object detection.
Semantics:
""""""""""

The result of ``ptrmask(ptr, mask)`` is equivalent to
``getelementptr ptr, (ptrtoint(ptr) & mask) - ptrtoint(ptr)``. Both the returned
pointer(s) and the first argument are based on the same underlying object (for more
information on the *based on* terminology see
The result of ``ptrmask(%ptr, %mask)`` is equivalent to the following expansion,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this actually true (both before and after)? I would hope that ptrmask does not have the address-exposed semantics that ptrtoint does?

Copy link
Contributor Author

@nikic nikic Oct 19, 2023 •
edited

Choose a reason for hiding this comment

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

I've added a sentence to clarify that ptrmask only captures via the return value.

where ``iPtrIdx`` is the index type size of the pointer::
arichardson marked this conversation as resolved.
Show resolved Hide resolved

%intptr = ptrtoint ptr %ptr to iPtrIdx ; this may truncate
%masked = and iPtrIdx %intptr, %mask
%diff = sub iPtrIdx %masked, %intptr
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this expansion works if the result of the sub is positive, does it?

Copy link
Member

Choose a reason for hiding this comment

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

But %diff will always be negative right? %masked must be less than %intptr since it's the result of an and.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the case where the subtraction overflows so the sign bit of %diff is 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you maybe have an example of what could go wrong here? I don't get it.

The sub here gets cancelled out by the add the GEP does, I don't think overflow is relevant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, it works specifically because everything is using iPtrIdx arithmetic, and therefore (in two's complement) does not matter whether things are positive or negative, it's just modular arithmetic

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of an example like this, with 16-bit pointers and 8-bit indexes:

  %ptr = ptr 0x6789
  %mask = i8 0x00
  // ptrmask(%ptr, %mask) expands to:
  %intptr = i8 0x89
  %masked = i8 0x00
  %diff = i8 0x77
  %result = getelementptr i8, ptr 0x6789, i8 0x77
          = ptr 0x6800 // not 0x6700!

Have I misunderstood how getelementptr works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getelementptr only works on the index bits of the pointer. So the result would be 0x6700, not 0x6800.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK but I don't see that documented in the semantics: https://llvm.org/docs/LangRef.html#id234

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please update the documentation of the semantics of getelementptr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as part of #70015.

%result = getelementptr i8, ptr %ptr, iPtrIdx %diff

Considering this as an operation on the integer representation of the pointer,
if the pointer index type size is smaller than the pointer type size, this
implies that the mask is extended with 1 bits to the pointer type size.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if the pointer index type size is smaller than the pointer type size, this
implies that the mask is extended with 1 bits to the pointer type size.
if the pointer index type size is smaller than the pointer type size, this
implies pointer bits beyond the index size are not affected by this intrinsic
(as if the mask was extended with 1 bits to the pointer type size for integral pointers).

Maybe something like this addresses @jrtc27's comment that non-index bits are not touched at all for CHERI?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like that does indeed sound more appropriate for pointers that aren't just plain integers (though technically our capabilities do still report as being integral, since non-integral pointers are overly-restrictive in their semantics (they imply that ptrtoint gives an unstable value, but ours are as stable as any normal integral pointer)).


Both the returned pointer(s) and the first argument are based on the same
underlying object (for more information on the *based on* terminology see
:ref:`the pointer aliasing rules <pointeraliasing>`). If the bitwidth of the
mask argument does not match the pointer size of the target, the mask is
zero-extended or truncated accordingly.
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Analysis/ValueTracking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1637,8 +1637,8 @@ static void computeKnownBitsFromOperator(const Operator *I,
const Value *Mask = I->getOperand(1);
Known2 = KnownBits(Mask->getType()->getScalarSizeInBits());
computeKnownBits(Mask, Known2, Depth + 1, Q);
// This is basically a pointer typed and.
Known &= Known2.zextOrTrunc(Known.getBitWidth());
// TODO: 1-extend would be more precise.
Known &= Known2.anyextOrTrunc(BitWidth);
break;
}
case Intrinsic::x86_sse42_crc32_64_64:
Expand Down
7 changes: 4 additions & 3 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7420,11 +7420,12 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
}
case Intrinsic::ptrmask: {
SDValue Ptr = getValue(I.getOperand(0));
SDValue Const = getValue(I.getOperand(1));
SDValue Mask = getValue(I.getOperand(1));

EVT PtrVT = Ptr.getValueType();
setValue(&I, DAG.getNode(ISD::AND, sdl, PtrVT, Ptr,
DAG.getZExtOrTrunc(Const, sdl, PtrVT)));
assert(PtrVT == Mask.getValueType() &&
arichardson marked this conversation as resolved.
Show resolved Hide resolved
"Pointers with different index type are not supported by SDAG");
setValue(&I, DAG.getNode(ISD::AND, sdl, PtrVT, Ptr, Mask));
return;
}
case Intrinsic::threadlocal_address: {
Expand Down
4 changes: 4 additions & 0 deletions llvm/lib/IR/Verifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5964,6 +5964,10 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
"llvm.ptrmask intrinsic arguments must have the same number of "
"elements",
&Call);
Check(DL.getIndexTypeSizeInBits(Ty0) == Ty1->getScalarSizeInBits(),
"llvm.ptrmask intrinsic second argument bitwidth must match "
"pointer index type size of first argument",
&Call);
break;
}
};
Expand Down
13 changes: 6 additions & 7 deletions llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1966,13 +1966,12 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
if (match(II->getArgOperand(0),
m_OneUse(m_Intrinsic<Intrinsic::ptrmask>(m_Value(InnerPtr),
m_Value(InnerMask))))) {
if (II->getArgOperand(1)->getType() == InnerMask->getType()) {
Value *NewMask = Builder.CreateAnd(II->getArgOperand(1), InnerMask);
return replaceInstUsesWith(
*II,
Builder.CreateIntrinsic(InnerPtr->getType(), Intrinsic::ptrmask,
{InnerPtr, NewMask}));
}
assert(II->getArgOperand(1)->getType() == InnerMask->getType() &&
"Mask types must match");
Value *NewMask = Builder.CreateAnd(II->getArgOperand(1), InnerMask);
return replaceInstUsesWith(
*II, Builder.CreateIntrinsic(InnerPtr->getType(), Intrinsic::ptrmask,
{InnerPtr, NewMask}));
}
break;
}
Expand Down
15 changes: 0 additions & 15 deletions llvm/test/CodeGen/AArch64/lower-ptrmask.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,3 @@ define ptr @test1(ptr %src) {
%ptr = call ptr @llvm.ptrmask.p0.i64(ptr %src, i64 72057594037927928)
ret ptr %ptr
}

declare ptr @llvm.ptrmask.p0.i32(ptr, i32)

; CHECK-LABEL: name: test2
; CHECK: %0:gpr64 = COPY $x0
; CHECK-NEXT: %1:gpr32 = MOVi32imm 10000
; CHECK-NEXT: %2:gpr64 = SUBREG_TO_REG 0, killed %1, %subreg.sub_32
; CHECK-NEXT: %3:gpr64 = ANDXrr %0, killed %2
; CHECK-NEXT: $x0 = COPY %3
; CHECK-NEXT: RET_ReallyLR implicit $x0

define ptr @test2(ptr %src) {
%ptr = call ptr @llvm.ptrmask.p0.i32(ptr %src, i32 10000)
ret ptr %ptr
}
108 changes: 0 additions & 108 deletions llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-ptrmask.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,78 +21,6 @@ define ptr @ptrmask_flat_i64(ptr %ptr, i64 %mask) {
ret ptr %masked
}

define ptr @ptrmask_flat_i32(ptr %ptr, i32 %mask) {
; CHECK-LABEL: name: ptrmask_flat_i32
; CHECK: bb.1 (%ir-block.0):
; CHECK-NEXT: liveins: $vgpr0, $vgpr1, $vgpr2
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $vgpr0
; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr1
; CHECK-NEXT: [[MV:%[0-9]+]]:_(p0) = G_MERGE_VALUES [[COPY]](s32), [[COPY1]](s32)
; CHECK-NEXT: [[COPY2:%[0-9]+]]:_(s32) = COPY $vgpr2
; CHECK-NEXT: [[PTRMASK:%[0-9]+]]:_(p0) = G_PTRMASK [[MV]], [[COPY2]](s32)
; CHECK-NEXT: [[UV:%[0-9]+]]:_(s32), [[UV1:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES [[PTRMASK]](p0)
; CHECK-NEXT: $vgpr0 = COPY [[UV]](s32)
; CHECK-NEXT: $vgpr1 = COPY [[UV1]](s32)
; CHECK-NEXT: SI_RETURN implicit $vgpr0, implicit $vgpr1
%masked = call ptr @llvm.ptrmask.p0.i32(ptr %ptr, i32 %mask)
ret ptr %masked
}

define ptr @ptrmask_flat_i16(ptr %ptr, i16 %mask) {
; CHECK-LABEL: name: ptrmask_flat_i16
; CHECK: bb.1 (%ir-block.0):
; CHECK-NEXT: liveins: $vgpr0, $vgpr1, $vgpr2
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $vgpr0
; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr1
; CHECK-NEXT: [[MV:%[0-9]+]]:_(p0) = G_MERGE_VALUES [[COPY]](s32), [[COPY1]](s32)
; CHECK-NEXT: [[COPY2:%[0-9]+]]:_(s32) = COPY $vgpr2
; CHECK-NEXT: [[TRUNC:%[0-9]+]]:_(s16) = G_TRUNC [[COPY2]](s32)
; CHECK-NEXT: [[PTRMASK:%[0-9]+]]:_(p0) = G_PTRMASK [[MV]], [[TRUNC]](s16)
; CHECK-NEXT: [[UV:%[0-9]+]]:_(s32), [[UV1:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES [[PTRMASK]](p0)
; CHECK-NEXT: $vgpr0 = COPY [[UV]](s32)
; CHECK-NEXT: $vgpr1 = COPY [[UV1]](s32)
; CHECK-NEXT: SI_RETURN implicit $vgpr0, implicit $vgpr1
%masked = call ptr @llvm.ptrmask.p0.i16(ptr %ptr, i16 %mask)
ret ptr %masked
}

define ptr @ptrmask_flat_i1(ptr %ptr, i1 %mask) {
; CHECK-LABEL: name: ptrmask_flat_i1
; CHECK: bb.1 (%ir-block.0):
; CHECK-NEXT: liveins: $vgpr0, $vgpr1, $vgpr2
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $vgpr0
; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr1
; CHECK-NEXT: [[MV:%[0-9]+]]:_(p0) = G_MERGE_VALUES [[COPY]](s32), [[COPY1]](s32)
; CHECK-NEXT: [[COPY2:%[0-9]+]]:_(s32) = COPY $vgpr2
; CHECK-NEXT: [[TRUNC:%[0-9]+]]:_(s1) = G_TRUNC [[COPY2]](s32)
; CHECK-NEXT: [[PTRMASK:%[0-9]+]]:_(p0) = G_PTRMASK [[MV]], [[TRUNC]](s1)
; CHECK-NEXT: [[UV:%[0-9]+]]:_(s32), [[UV1:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES [[PTRMASK]](p0)
; CHECK-NEXT: $vgpr0 = COPY [[UV]](s32)
; CHECK-NEXT: $vgpr1 = COPY [[UV1]](s32)
; CHECK-NEXT: SI_RETURN implicit $vgpr0, implicit $vgpr1
%masked = call ptr @llvm.ptrmask.p0.i1(ptr %ptr, i1 %mask)
ret ptr %masked
}

define ptr addrspace(3) @ptrmask_local_i64(ptr addrspace(3) %ptr, i64 %mask) {
; CHECK-LABEL: name: ptrmask_local_i64
; CHECK: bb.1 (%ir-block.0):
; CHECK-NEXT: liveins: $vgpr0, $vgpr1, $vgpr2
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(p3) = COPY $vgpr0
; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr1
; CHECK-NEXT: [[COPY2:%[0-9]+]]:_(s32) = COPY $vgpr2
; CHECK-NEXT: [[MV:%[0-9]+]]:_(s64) = G_MERGE_VALUES [[COPY1]](s32), [[COPY2]](s32)
; CHECK-NEXT: [[PTRMASK:%[0-9]+]]:_(p3) = G_PTRMASK [[COPY]], [[MV]](s64)
; CHECK-NEXT: $vgpr0 = COPY [[PTRMASK]](p3)
; CHECK-NEXT: SI_RETURN implicit $vgpr0
%masked = call ptr addrspace(3) @llvm.ptrmask.p3.i64(ptr addrspace(3) %ptr, i64 %mask)
ret ptr addrspace(3) %masked
}

define ptr addrspace(3) @ptrmask_local_i32(ptr addrspace(3) %ptr, i32 %mask) {
; CHECK-LABEL: name: ptrmask_local_i32
; CHECK: bb.1 (%ir-block.0):
Expand All @@ -107,47 +35,11 @@ define ptr addrspace(3) @ptrmask_local_i32(ptr addrspace(3) %ptr, i32 %mask) {
ret ptr addrspace(3) %masked
}

define ptr addrspace(3) @ptrmask_local_i16(ptr addrspace(3) %ptr, i16 %mask) {
; CHECK-LABEL: name: ptrmask_local_i16
; CHECK: bb.1 (%ir-block.0):
; CHECK-NEXT: liveins: $vgpr0, $vgpr1
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(p3) = COPY $vgpr0
; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr1
; CHECK-NEXT: [[TRUNC:%[0-9]+]]:_(s16) = G_TRUNC [[COPY1]](s32)
; CHECK-NEXT: [[PTRMASK:%[0-9]+]]:_(p3) = G_PTRMASK [[COPY]], [[TRUNC]](s16)
; CHECK-NEXT: $vgpr0 = COPY [[PTRMASK]](p3)
; CHECK-NEXT: SI_RETURN implicit $vgpr0
%masked = call ptr addrspace(3) @llvm.ptrmask.p3.i16(ptr addrspace(3) %ptr, i16 %mask)
ret ptr addrspace(3) %masked
}

define ptr addrspace(3) @ptrmask_local_i1(ptr addrspace(3) %ptr, i1 %mask) {
; CHECK-LABEL: name: ptrmask_local_i1
; CHECK: bb.1 (%ir-block.0):
; CHECK-NEXT: liveins: $vgpr0, $vgpr1
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:_(p3) = COPY $vgpr0
; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr1
; CHECK-NEXT: [[TRUNC:%[0-9]+]]:_(s1) = G_TRUNC [[COPY1]](s32)
; CHECK-NEXT: [[PTRMASK:%[0-9]+]]:_(p3) = G_PTRMASK [[COPY]], [[TRUNC]](s1)
; CHECK-NEXT: $vgpr0 = COPY [[PTRMASK]](p3)
; CHECK-NEXT: SI_RETURN implicit $vgpr0
%masked = call ptr addrspace(3) @llvm.ptrmask.p3.i1(ptr addrspace(3) %ptr, i1 %mask)
ret ptr addrspace(3) %masked
}

; Seems to not work
; define <2 x ptr> @ptrmask_flat_i64_v2(<2 x ptr> %ptr, <2 x i64> %mask) {
; %masked = call <2 x ptr> @llvm.ptrmask.v2p0.v2i64(<2 x ptr> %ptr, <2 x i64> %mask)
; ret <2 x ptr> %masked
; }

declare ptr @llvm.ptrmask.p0.i64(ptr, i64)
declare ptr @llvm.ptrmask.p0.i32(ptr, i32)
declare ptr @llvm.ptrmask.p0.i16(ptr, i16)
declare ptr @llvm.ptrmask.p0.i1(ptr, i1)
declare ptr addrspace(3) @llvm.ptrmask.p3.i64(ptr addrspace(3), i64)
declare ptr addrspace(3) @llvm.ptrmask.p3.i32(ptr addrspace(3), i32)
declare ptr addrspace(3) @llvm.ptrmask.p3.i16(ptr addrspace(3), i16)
declare ptr addrspace(3) @llvm.ptrmask.p3.i1(ptr addrspace(3), i1)