Doesn't the 'uitofp' conversion imply that we know that %conv is >= 0? I think this transformation is correct.

I also think this looks correct. How did you run into this?

My apologies.. looks like the code generated is correct.

ran into a problem with this patch when an application stopped working and the corresponding source code is as follow.

      T* lbp = bp + 1;
      double lbp1 = (double)(size_t)(lbp);
      if( lbp1 > 0.0 )
      {
         delete[]( lbp );

somehow above comparision code is transformed to following after instruction combining after this patch. 

 br i1 true, label %if.then, label %if.end, 

as noted, there is no comparision check  lpb1 > 0.0 and delete of lbp is being called when lpb1 is 0.

will try to get simplified testcase for above.

Created attachment 13967 [details]
input c++ program

Created attachment 13968 [details]
input to opt

Created attachment 13969 [details]
output of opt

The test input c++ program is attached.

as seen, the input is 
     int* lbp = bp + 1;
     double lbp1 = (double) (size_t)(lbp);
     if( lbp1 > 0.0 )
     {

the pointer bp is initialized to value "-1" outside the function and the array starts from index 1.

the explicit conversion to double in the statement

     double lbp1 = (double) (size_t)(lbp);

is removed in the llvm patch.this changes the comparison to unsigned comparision which removes the check for lbp1 > 0.0. This will make the delete to be called for 0th element, which is not initialized.

from my perspective, the source code seem to be OK. let me know otherwise.

The optimization seems correct because we are converting an unsigned value to a double.  The fact that the underlying bit-pattern is -1 is not really relevant.

(In reply to comment #8)
> The optimization seems correct because we are converting an unsigned value
> to a double.  The fact that the underlying bit-pattern is -1 is not really
> relevant.

Right, when you do the conversion, you end up with UINT_MAX, not -1. That's always bigger than 0.

In the case I mentioned, bp is -1 and lbp is 0

     int* lbp = bp + 1;     // bp = -1, lbp = 0
     double lbp1 = (double) (size_t)(lbp); // lbp1=0
     if( lbp1 > 0.0 )  //  condition is removed
     {

Agree that lbp1 can not be negative, but  it can be 0  and the condition check should not have been removed.

(In reply to comment #10)
> Agree that lbp1 can not be negative, but  it can be 0  and the condition
> check should not have been removed.

According to your original report, the condition hasn't been removed:
  %i = ptrtoint i64* %add.ptr to i64
  %cmp = icmp eq i64 %i, 0
  br i1 %cmp, label %if.else, label %if.then

The icmp eq i64 0 is still there.

Yes. Apologies for that, as it is incorrect test. Please look into comment #4 and the attached testcase.

I looked into this bug and try to dig into code base.

For the test case:

int foo(int *bp)
{
     int* lbp = bp + 1;
     double lbp1 = (double) (size_t)(lbp);
      if( lbp1 > 0.0 )
      {
         delete[]( lbp );
      }
      return 0;
}

IR before instcombine:

define i32 @_Z3fooPi(i32* %bp) #0 {
entry:
  %add.ptr = getelementptr inbounds i32, i32* %bp, i64 1
  %0 = ptrtoint i32* %add.ptr to i64
  %conv = uitofp i64 %0 to double
  %cmp = fcmp ogt double %conv, 0.000000e+00
  br i1 %cmp, label %if.then, label %if.end

if.then:                                         
  %isnull = icmp eq i32* %add.ptr, null
  br i1 %isnull, label %delete.end, label %delete.notnull

delete.notnull:                                  
  %1 = bitcast i32* %add.ptr to i8*
  call void @_ZdaPv(i8* %1) #2
  br label %delete.end

delete.end:                                       
 %if.then
  br label %if.end

if.end:                                           
  ret i32 0
}
 
Matt's checkin converts "fcmp ogt" to "icmp ugt", which is correct.

IR of comparison after conversion.

  %add.ptr = getelementptr inbounds i32, i32* %bp, i64 1
  %0 = ptrtoint i32* %add.ptr to i64
  %cmp = icmp ugt i64 %0, 0

Now further, the code again tries instruction combining in -O2 and as a part of it, instruction simplification runs on "icmp ugt" test case.

In instruction simplify, there is a call to isKnownNonZero(), which checks if %0 is non-zero. %0 is pointer to integer conversion. So, %add.ptr is checked if its non-null by isGEPKnownNonNull(). The piece of code which does this in ValueTracking.cpp file :

  if (V->getType()->isPointerTy()) {
      if (isKnownNonNull(V))
        return true; 
      if (GEPOperator *GEP = dyn_cast<GEPOperator>(V))
        if (isGEPKnownNonNull(GEP, DL, Depth, Q))
          return true;
    }

The isGEPKnownNonNull() walks the GEP operands and see if any operand introduces a non-zero offset. If so, then the GEP cannot produce a null pointer.
So it checks the index operand of the GEP, which is 1 (+ve, not zero) and hence return true.

  if (ConstantInt *OpC = dyn_cast<ConstantInt>(GTI.getOperand())) {
        if (!OpC->isZero())
          return true;
  }

The code seems to produce correct code. 

However, Lets assume that in the original test case, bp was having INT_MAX value. Adding 1 to it will make it 0. 

       int* lbp = bp + 1;

So, lbp should be 0 here. After converting it to unsigned int and double, lbp should be 0.0. So > 0.0 must be false, which is not the case with the resultant IR. The resultant IR :

  %add.ptr = getelementptr inbounds i32, i32* %bp, i64 1
  br i1 true, label %if.then, label %if.end

So the comparision is always true which might not be the case always. Isn't this a bug? Can someone confirm on this? Am i missing something?

> However, Lets assume that in the original test case, bp was having INT_MAX value. Adding 1 to it will make it 0. 
> 
>       int* lbp = bp + 1;

According to the semantics of getelementptr inbounds, lbp is poison if bp was INT_MAX.

Alive2 shows that the transformation is correct..! :) https://alive2.llvm.org/ce/z/AGEgCL