[llvm] f08148e - [ConstProp] Fix folding of pointer icmp with signed predicates
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 8 09:54:16 PST 2021
On Mon, Mar 8, 2021 at 6:26 PM Philip Reames <listmail at philipreames.com>
wrote:
> Nikita,
>
> At least on common platforms, use programs can't place globals in the
> top half of the address space. Generally, the high bit being one means
> "kernel space".
>
> We probably shouldn't bake that assumption in outright - otherwise,
> compiling the kernel becomes "fun" - but it'd be nice not to loose it
> entirely. Is there a configuration flag we can base this off?
>
> Philip
>
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).
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.
Regards,
Nikita
> On 3/8/21 8:14 AM, Nikita Popov via llvm-commits wrote:
> > Author: Nikita Popov
> > Date: 2021-03-08T17:12:12+01:00
> > New Revision: f08148e874088a07b972203a183db00de9c38a70
> >
> > URL:
> https://github.com/llvm/llvm-project/commit/f08148e874088a07b972203a183db00de9c38a70
> > DIFF:
> https://github.com/llvm/llvm-project/commit/f08148e874088a07b972203a183db00de9c38a70.diff
> >
> > LOG: [ConstProp] Fix folding of pointer icmp with signed predicates
> >
> > While @g ugt null is always true (ignoring weak symbols),
> > @g sgt null is not necessarily the case -- that would imply that
> > it is forbidden to place globals in the high half of the address
> > space.
> >
> > Added:
> >
> >
> > Modified:
> > llvm/lib/IR/ConstantFold.cpp
> > llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll
> >
> > Removed:
> >
> >
> >
> >
> ################################################################################
> > diff --git a/llvm/lib/IR/ConstantFold.cpp b/llvm/lib/IR/ConstantFold.cpp
> > index 3903d72ac7c3..2e0ea4c50e79 100644
> > --- a/llvm/lib/IR/ConstantFold.cpp
> > +++ b/llvm/lib/IR/ConstantFold.cpp
> > @@ -1838,35 +1838,27 @@ static ICmpInst::Predicate
> evaluateICmpRelation(Constant *V1, Constant *V2,
> > // If we are comparing a GEP to a null pointer, check to see
> if the base
> > // of the GEP equals the null pointer.
> > if (const GlobalValue *GV = dyn_cast<GlobalValue>(CE1Op0)) {
> > - if (GV->hasExternalWeakLinkage())
> > - // Weak linkage GVals could be zero or not. We're comparing
> that
> > - // to null pointer so its greater-or-equal
> > - return isSigned ? ICmpInst::ICMP_SGE : ICmpInst::ICMP_UGE;
> > - else
> > - // If its not weak linkage, the GVal must have a non-zero
> address
> > - // so the result is greater-than
> > - return isSigned ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT;
> > + // If its not weak linkage, the GVal must have a non-zero
> address
> > + // so the result is greater-than
> > + if (!GV->hasExternalWeakLinkage())
> > + return ICmpInst::ICMP_UGT;
> > } else if (isa<ConstantPointerNull>(CE1Op0)) {
> > // If we are indexing from a null pointer, check to see if
> we have any
> > // non-zero indices.
> > for (unsigned i = 1, e = CE1->getNumOperands(); i != e; ++i)
> > if (!CE1->getOperand(i)->isNullValue())
> > // Offsetting from null, must not be equal.
> > - return isSigned ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT;
> > + return ICmpInst::ICMP_UGT;
> > // Only zero indexes from null, must still be zero.
> > return ICmpInst::ICMP_EQ;
> > }
> > // Otherwise, we can't really say if the first operand is null
> or not.
> > } else if (const GlobalValue *GV2 = dyn_cast<GlobalValue>(V2)) {
> > if (isa<ConstantPointerNull>(CE1Op0)) {
> > - if (GV2->hasExternalWeakLinkage())
> > - // Weak linkage GVals could be zero or not. We're comparing
> it to
> > - // a null pointer, so its less-or-equal
> > - return isSigned ? ICmpInst::ICMP_SLE : ICmpInst::ICMP_ULE;
> > - else
> > - // If its not weak linkage, the GVal must have a non-zero
> address
> > - // so the result is less-than
> > - return isSigned ? ICmpInst::ICMP_SLT : ICmpInst::ICMP_ULT;
> > + // If its not weak linkage, the GVal must have a non-zero
> address
> > + // so the result is less-than
> > + if (!GV2->hasExternalWeakLinkage())
> > + return ICmpInst::ICMP_ULT;
> > } else if (const GlobalValue *GV =
> dyn_cast<GlobalValue>(CE1Op0)) {
> > if (GV == GV2) {
> > // If this is a getelementptr of the same global, then it
> must be
> > @@ -1876,7 +1868,7 @@ static ICmpInst::Predicate
> evaluateICmpRelation(Constant *V1, Constant *V2,
> > assert(CE1->getNumOperands() == 2 &&
> > !CE1->getOperand(1)->isNullValue() &&
> > "Surprising getelementptr!");
> > - return isSigned ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT;
> > + return ICmpInst::ICMP_UGT;
> > } else {
> > if (CE1GEP->hasAllZeroIndices())
> > return areGlobalsPotentiallyEqual(GV, GV2);
> >
> > diff --git a/llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll
> b/llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll
> > index 46fd8c4bfce8..41a64205633e 100644
> > --- a/llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll
> > +++ b/llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll
> > @@ -110,7 +110,7 @@ define i1 @global_gep_ugt_null() {
> >
> > define i1 @global_gep_sgt_null() {
> > ; CHECK-LABEL: @global_gep_sgt_null(
> > -; CHECK-NEXT: ret i1 true
> > +; CHECK-NEXT: ret i1 icmp sgt ([2 x i32]* getelementptr inbounds ([2
> x i32], [2 x i32]* @g, i64 1), [2 x i32]* null)
> > ;
> > %gep = getelementptr inbounds [2 x i32], [2 x i32]* @g, i64 1
> > %cmp = icmp sgt [2 x i32]* %gep, null
> > @@ -137,7 +137,7 @@ define i1 @null_gep_ugt_null() {
> >
> > define i1 @null_gep_sgt_null() {
> > ; CHECK-LABEL: @null_gep_sgt_null(
> > -; CHECK-NEXT: ret i1 true
> > +; CHECK-NEXT: ret i1 icmp sgt (i8* getelementptr (i8, i8* null, i64
> ptrtoint (i32* @g2 to i64)), i8* null)
> > ;
> > %gep = getelementptr i8, i8* null, i64 ptrtoint (i32* @g2 to i64)
> > %cmp = icmp sgt i8* %gep, null
> > @@ -164,7 +164,7 @@ define i1 @null_gep_ult_global() {
> >
> > define i1 @null_gep_slt_global() {
> > ; CHECK-LABEL: @null_gep_slt_global(
> > -; CHECK-NEXT: ret i1 true
> > +; 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)
> > ;
> > %gep = getelementptr [2 x i32], [2 x i32]* null, i64 ptrtoint (i32*
> @g2 to i64)
> > %cmp = icmp slt [2 x i32]* %gep, @g
> > @@ -191,7 +191,7 @@ define i1 @global_gep_ugt_global() {
> >
> > define i1 @global_gep_sgt_global() {
> > ; CHECK-LABEL: @global_gep_sgt_global(
> > -; CHECK-NEXT: ret i1 true
> > +; CHECK-NEXT: ret i1 icmp sgt ([2 x i32]* getelementptr inbounds ([2
> x i32], [2 x i32]* @g, i64 1), [2 x i32]* @g)
> > ;
> > %gep = getelementptr inbounds [2 x i32], [2 x i32]* @g, i64 1
> > %cmp = icmp sgt [2 x i32]* %gep, @g
> > @@ -209,7 +209,7 @@ define i1 @global_gep_ugt_global_neg_offset() {
> >
> > define i1 @global_gep_sgt_global_neg_offset() {
> > ; CHECK-LABEL: @global_gep_sgt_global_neg_offset(
> > -; CHECK-NEXT: ret i1 true
> > +; CHECK-NEXT: ret i1 icmp sgt ([2 x i32]* getelementptr ([2 x i32],
> [2 x i32]* @g, i64 -1), [2 x i32]* @g)
> > ;
> > %gep = getelementptr [2 x i32], [2 x i32]* @g, i64 -1
> > %cmp = icmp sgt [2 x i32]* %gep, @g
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210308/c1840437/attachment.html>
More information about the llvm-commits
mailing list