<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 3/8/21 9:54 AM, Nikita Popov wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF+90c-WUJ3HfgRmLhwNMEWokZSYMnMgmONO04CGPzRCM9xE-Q@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Mon, Mar 8, 2021 at 6:26
PM Philip Reames <<a
href="mailto:listmail@philipreames.com"
moz-do-not-send="true">listmail@philipreames.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">Nikita,<br>
<br>
At least on common platforms, use programs can't place
globals in the <br>
top half of the address space. Generally, the high bit
being one means <br>
"kernel space".<br>
<br>
We probably shouldn't bake that assumption in outright -
otherwise, <br>
compiling the kernel becomes "fun" - but it'd be nice not to
loose it <br>
entirely. Is there a configuration flag we can base this
off?<br>
<br>
Philip<br>
</blockquote>
<div><br>
</div>
<div>As least I'm not aware of any -- as far as I know the
only flag we have is whether objects may start at a null
pointer (which, incidentally, is another thing this code
handles incorrectly).</div>
<div><br>
</div>
<div>I don't think it's particularly useful to preserve this
fold even on platforms where it would be correct, as
frontends generally emit pointer comparisons as unsigned
(including clang), which makes it unlikely for these to
occur in the wild. As far as I can tell, this code dates all
the way back to when LLVM switched away from signed/unsigned
types in favor of signed/unsigned comparisons. The original
code was (implicitly) unsigned, and ended up handling both
signed and unsigned after that migration.</div>
</div>
</div>
</blockquote>
Fair. I don't care strongly, we can revisit if it ever comes up in
the future.<br>
<blockquote type="cite"
cite="mid:CAF+90c-WUJ3HfgRmLhwNMEWokZSYMnMgmONO04CGPzRCM9xE-Q@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>Regards,<br>
</div>
<div>Nikita<br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
On 3/8/21 8:14 AM, Nikita Popov via llvm-commits wrote:<br>
> Author: Nikita Popov<br>
> Date: 2021-03-08T17:12:12+01:00<br>
> New Revision: f08148e874088a07b972203a183db00de9c38a70<br>
><br>
> URL: <a
href="https://github.com/llvm/llvm-project/commit/f08148e874088a07b972203a183db00de9c38a70"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://github.com/llvm/llvm-project/commit/f08148e874088a07b972203a183db00de9c38a70</a><br>
> DIFF: <a
href="https://github.com/llvm/llvm-project/commit/f08148e874088a07b972203a183db00de9c38a70.diff"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://github.com/llvm/llvm-project/commit/f08148e874088a07b972203a183db00de9c38a70.diff</a><br>
><br>
> LOG: [ConstProp] Fix folding of pointer icmp with
signed predicates<br>
><br>
> While @g ugt null is always true (ignoring weak
symbols),<br>
> @g sgt null is not necessarily the case -- that would
imply that<br>
> it is forbidden to place globals in the high half of
the address<br>
> space.<br>
><br>
> Added:<br>
> <br>
><br>
> Modified:<br>
> llvm/lib/IR/ConstantFold.cpp<br>
>
llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll<br>
><br>
> Removed:<br>
> <br>
><br>
><br>
>
################################################################################<br>
> diff --git a/llvm/lib/IR/ConstantFold.cpp
b/llvm/lib/IR/ConstantFold.cpp<br>
> index 3903d72ac7c3..2e0ea4c50e79 100644<br>
> --- a/llvm/lib/IR/ConstantFold.cpp<br>
> +++ b/llvm/lib/IR/ConstantFold.cpp<br>
> @@ -1838,35 +1838,27 @@ static ICmpInst::Predicate
evaluateICmpRelation(Constant *V1, Constant *V2,<br>
> // If we are comparing a GEP to a null
pointer, check to see if the base<br>
> // of the GEP equals the null pointer.<br>
> if (const GlobalValue *GV =
dyn_cast<GlobalValue>(CE1Op0)) {<br>
> - if (GV->hasExternalWeakLinkage())<br>
> - // Weak linkage GVals could be zero or
not. We're comparing that<br>
> - // to null pointer so its greater-or-equal<br>
> - return isSigned ? ICmpInst::ICMP_SGE :
ICmpInst::ICMP_UGE;<br>
> - else<br>
> - // If its not weak linkage, the GVal must
have a non-zero address<br>
> - // so the result is greater-than<br>
> - return isSigned ? ICmpInst::ICMP_SGT :
ICmpInst::ICMP_UGT;<br>
> + // If its not weak linkage, the GVal must
have a non-zero address<br>
> + // so the result is greater-than<br>
> + if (!GV->hasExternalWeakLinkage())<br>
> + return ICmpInst::ICMP_UGT;<br>
> } else if
(isa<ConstantPointerNull>(CE1Op0)) {<br>
> // If we are indexing from a null pointer,
check to see if we have any<br>
> // non-zero indices.<br>
> for (unsigned i = 1, e =
CE1->getNumOperands(); i != e; ++i)<br>
> if
(!CE1->getOperand(i)->isNullValue())<br>
> // Offsetting from null, must not be
equal.<br>
> - return isSigned ? ICmpInst::ICMP_SGT :
ICmpInst::ICMP_UGT;<br>
> + return ICmpInst::ICMP_UGT;<br>
> // Only zero indexes from null, must still
be zero.<br>
> return ICmpInst::ICMP_EQ;<br>
> }<br>
> // Otherwise, we can't really say if the
first operand is null or not.<br>
> } else if (const GlobalValue *GV2 =
dyn_cast<GlobalValue>(V2)) {<br>
> if (isa<ConstantPointerNull>(CE1Op0)) {<br>
> - if (GV2->hasExternalWeakLinkage())<br>
> - // Weak linkage GVals could be zero or
not. We're comparing it to<br>
> - // a null pointer, so its less-or-equal<br>
> - return isSigned ? ICmpInst::ICMP_SLE :
ICmpInst::ICMP_ULE;<br>
> - else<br>
> - // If its not weak linkage, the GVal must
have a non-zero address<br>
> - // so the result is less-than<br>
> - return isSigned ? ICmpInst::ICMP_SLT :
ICmpInst::ICMP_ULT;<br>
> + // If its not weak linkage, the GVal must
have a non-zero address<br>
> + // so the result is less-than<br>
> + if (!GV2->hasExternalWeakLinkage())<br>
> + return ICmpInst::ICMP_ULT;<br>
> } else if (const GlobalValue *GV =
dyn_cast<GlobalValue>(CE1Op0)) {<br>
> if (GV == GV2) {<br>
> // If this is a getelementptr of the same
global, then it must be<br>
> @@ -1876,7 +1868,7 @@ static ICmpInst::Predicate
evaluateICmpRelation(Constant *V1, Constant *V2,<br>
> assert(CE1->getNumOperands() == 2
&&<br>
>
!CE1->getOperand(1)->isNullValue() &&<br>
> "Surprising getelementptr!");<br>
> - return isSigned ? ICmpInst::ICMP_SGT :
ICmpInst::ICMP_UGT;<br>
> + return ICmpInst::ICMP_UGT;<br>
> } else {<br>
> if (CE1GEP->hasAllZeroIndices())<br>
> return areGlobalsPotentiallyEqual(GV,
GV2);<br>
><br>
> diff --git
a/llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll
b/llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll<br>
> index 46fd8c4bfce8..41a64205633e 100644<br>
> ---
a/llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll<br>
> +++
b/llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll<br>
> @@ -110,7 +110,7 @@ define i1 @global_gep_ugt_null() {<br>
> <br>
> define i1 @global_gep_sgt_null() {<br>
> ; CHECK-LABEL: @global_gep_sgt_null(<br>
> -; CHECK-NEXT: ret i1 true<br>
> +; CHECK-NEXT: ret i1 icmp sgt ([2 x i32]*
getelementptr inbounds ([2 x i32], [2 x i32]* @g, i64 1), [2
x i32]* null)<br>
> ;<br>
> %gep = getelementptr inbounds [2 x i32], [2 x i32]*
@g, i64 1<br>
> %cmp = icmp sgt [2 x i32]* %gep, null<br>
> @@ -137,7 +137,7 @@ define i1 @null_gep_ugt_null() {<br>
> <br>
> define i1 @null_gep_sgt_null() {<br>
> ; CHECK-LABEL: @null_gep_sgt_null(<br>
> -; CHECK-NEXT: ret i1 true<br>
> +; CHECK-NEXT: ret i1 icmp sgt (i8* getelementptr
(i8, i8* null, i64 ptrtoint (i32* @g2 to i64)), i8* null)<br>
> ;<br>
> %gep = getelementptr i8, i8* null, i64 ptrtoint
(i32* @g2 to i64)<br>
> %cmp = icmp sgt i8* %gep, null<br>
> @@ -164,7 +164,7 @@ define i1 @null_gep_ult_global() {<br>
> <br>
> define i1 @null_gep_slt_global() {<br>
> ; CHECK-LABEL: @null_gep_slt_global(<br>
> -; CHECK-NEXT: ret i1 true<br>
> +; CHECK-NEXT: ret i1 icmp slt ([2 x i32]*
getelementptr ([2 x i32], [2 x i32]* null, i64 ptrtoint
(i32* @g2 to i64)), [2 x i32]* @g)<br>
> ;<br>
> %gep = getelementptr [2 x i32], [2 x i32]* null,
i64 ptrtoint (i32* @g2 to i64)<br>
> %cmp = icmp slt [2 x i32]* %gep, @g<br>
> @@ -191,7 +191,7 @@ define i1 @global_gep_ugt_global()
{<br>
> <br>
> define i1 @global_gep_sgt_global() {<br>
> ; CHECK-LABEL: @global_gep_sgt_global(<br>
> -; CHECK-NEXT: ret i1 true<br>
> +; CHECK-NEXT: ret i1 icmp sgt ([2 x i32]*
getelementptr inbounds ([2 x i32], [2 x i32]* @g, i64 1), [2
x i32]* @g)<br>
> ;<br>
> %gep = getelementptr inbounds [2 x i32], [2 x i32]*
@g, i64 1<br>
> %cmp = icmp sgt [2 x i32]* %gep, @g<br>
> @@ -209,7 +209,7 @@ define i1
@global_gep_ugt_global_neg_offset() {<br>
> <br>
> define i1 @global_gep_sgt_global_neg_offset() {<br>
> ; CHECK-LABEL: @global_gep_sgt_global_neg_offset(<br>
> -; CHECK-NEXT: ret i1 true<br>
> +; CHECK-NEXT: ret i1 icmp sgt ([2 x i32]*
getelementptr ([2 x i32], [2 x i32]* @g, i64 -1), [2 x i32]*
@g)<br>
> ;<br>
> %gep = getelementptr [2 x i32], [2 x i32]* @g, i64
-1<br>
> %cmp = icmp sgt [2 x i32]* %gep, @g<br>
><br>
><br>
> <br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org"
target="_blank" moz-do-not-send="true">llvm-commits@lists.llvm.org</a><br>
> <a
href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</div>
</blockquote>
</body>
</html>