<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">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><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">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">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">llvm-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>