[llvm-commits] Pointer subtraction for IRBuilder

[Nick Lewycky]

Nick Lewycky wrote:
> Jeffrey Yasskin wrote:
>> The attached file implements IRBuilder::CreatePSub(), which performs a
>> C-style pointer difference on its arguments. The included test passes,
>> but it involves a warning that "ISO C++ forbids casting between
>> pointer-to-function and pointer-to-object", which I don't know how to
>> get rid of given the ExecutionEngine interface. I was also hoping to
>> assert that constant folding could produce a constant integer, but I
>> couldn't figure that out.
> 
> Instead, construct the IRBuilder with a TargetData with known pointer 
> size and use Constants of pointer type (ie. "gep ptrtotint null, i32 1") 
> as the Values when calling CreatePSub. That way, you'll get a simple 
> Constant value back which must be a ConstantInt you can easily check.
> 
> Whatever you do, don't submit a 46 line test for a 7 line function. :)
> 
>> Let me know what you think. (And I'm now subscribed, so I'll get your answer.)
> 
> --- include/llvm/Support/IRBuilder.h	(revision 68715)
> +++ include/llvm/Support/IRBuilder.h	(working copy)
> @@ -681,8 +681,26 @@
>                           Name);
>     }
> +  /// CreatePSub - Return the i64 difference between two pointer 
> values, taking
> 
> The name CreatePSub is too ambiguous. Either CreatePtrSub or 
> CreatePtrDiff. I prefer the latter.
> 
> +  /// the size of the pointed-to objects into account.  This is intended to
> +  /// implement C's pointer subtraction.
> 
> "implement C-style pointer subtraction."
> 
> +  Value *CreatePSub(Value *LHS, Value *RHS, const char *Name = "");
>   };
> 
> +template <bool preserveNames, typename T>
> +Value *IRBuilder<preserveNames, T>::CreatePSub(
> +    Value *LHS, Value *RHS, const char *Name) {
> +  assert(LHS->getType() == RHS->getType() &&
> +         "Pointer subtraction operand types must match!");
> +  const PointerType *ArgType = cast<PointerType>(LHS->getType());
> 
> +  Value *LHS_int = CreatePtrToInt(LHS, Type::Int64Ty);
> +  Value *RHS_int = CreatePtrToInt(RHS, Type::Int64Ty);
> 
> I'm don't think this is safe. With TargetData, you could use 
> TD->getIntPtrType(). Without it, I'm not convinced it's safe even if you 
> were to assume that pointer size is <= 64 bits...
> 
> +  Value *Difference = CreateSub(LHS_int, RHS_int);
> +  return CreateSDiv(Difference,
> +                    ConstantExpr::getSizeOf(ArgType->getElementType()),
> +                    Name);
> 
> ...and here's where it would matter. Consider i32* inttoptr(i32 -1 to 
> i32*). CreatePtrToInt zero-extends to produce the i64 LHS_int or RHS_int.
> 
> On a 32-bit system:
>    int32_t *a = -1;
>    int32_t *b = -13;
>    a - b
> you would expand the -1 to 0x00000000ffffffff and the -13 to 
> 0x00000000fffffff3. b - a = 0xfffffffffffffff4, not 0x00000000fffffff4
> 
> 0xff...f4 sdiv 4 = i64 -3, while 0x00...0f...f4 sdiv 4 = 1073741821.

*Erbl.*

Which means of course that your method produces the correct answer, 
because you chose sdiv instead of udiv.

I got confused because inttoptr does zero-extension (implying that it's 
unsigned) but the division is signed.

Anyways, please discard the above misanalysis. I'm still suspicious of 
this code but I don't have example values of it misbehaving.

Sorry for the confusion!

Nick