[llvm] f08148e - [ConstProp] Fix folding of pointer icmp with signed predicates

Philip Reames via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 8 09:25:55 PST 2021


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

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


More information about the llvm-commits mailing list