[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 10:04:01 PST 2021
On 3/8/21 9:54 AM, Nikita Popov wrote:
> On Mon, Mar 8, 2021 at 6:26 PM Philip Reames
> <listmail at philipreames.com <mailto: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.
Fair. I don't care strongly, we can revisit if it ever comes up in the
future.
>
> 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
> <https://github.com/llvm/llvm-project/commit/f08148e874088a07b972203a183db00de9c38a70>
> > DIFF:
> 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 <mailto:llvm-commits at lists.llvm.org>
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> <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/5f7cf1e3/attachment.html>
More information about the llvm-commits
mailing list