[llvm-commits] Pointer subtraction for IRBuilder

[Nick Lewycky]

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.

So the fix is for this method to use TargetData.

Unless you want it to also work without TargetData -- now, this is 
hideous but functional -- you could fix it by truncating off the high 
bits of the subtraction result before the sdiv.

But how do you know how many bits to clear? You don't. The way to do it 
is to run it Different through an inttoptr/ptrtoint pair, because a 
pointer magically has the right number of bits.

Nick