Page MenuHomePhabricator

Add CreatePointerBitCastOrAddrSpaceCast to IRBuilder and co.
ClosedPublic

Authored by arsenm on Jan 22 2014, 3:07 PM.

Details

Reviewers
arsenm
Summary

This already exists in ConstantExpr, but the corresponding version everywhere else is missing.

Diff Detail

Event Timeline

Hi Matt,

Just to be sure, CreatePointerBitCastOrAddrSpaceCast and co are a subset of that is currently available through CreatePointerCast, right?
Apart from compile time improvement (if any) when we will use it, is there anything else we could expect from adding this API?

My point is do we really need a new API that may be confusing?
I haven’t find the discussion on why it was a good idea to add getPointerBitCastOrAddrSpaceCast for the constants in the first place, so I may just missing some context.

Sorry if I miss something obvious.

Thanks,
-Quentin

include/llvm/IR/IRBuilder.h
1191

No brackets for 1 line blocks.

arsenm added a comment.Mar 6 2014, 1:51 PM

The reasoning is that it is more restrictive than CreatePointerCast. CreatePointerCast will create a bitcast or addrspacecast depending on the source and destination address spaces. I've found that 90+% of the time, creating an addrspacecast is a bug from not using the correct address space for one of the operands. This makes it explicit that a different address space is OK in the relatively rare places where it is expected.

So if I understood correctly, this is syntactic sugar to help spotting the places where it was likely we were not expected an addrspace to be created.
However, CreatePointerCast producing an addrspace is still a valid option.

If you believe there is a value in making such a syntactic distinction, I suggest we do a bit of a refactoring in that patch.
Let us make CreatePointerCast and co. calls CreatePointerBitCastOrAddrSpace and co.
That way, we avoid the code duplication and we still have a way to make this distinction.

What do you think?

Thanks,
-Quentin

arsenm added a comment.Mar 6 2014, 2:55 PM

It's also more restrictive in that it doesn't allow ptrtoints. This has the same problem that I mentioned in D2205 when I added the constant version. The ptrtoint handling in CreatePointerCast would make using this in CreatePointerCast messy.

Let me be more specific on the proposal.

I think CastInst::CreatePointerCast can call CastInst::CreatePointerBitCastOrAddrSpaceCast as soon as we handled the PtrToInt part.
I.e.,
CastInst *CastInst::CreatePointerCast(Value *S, Type *Ty,

                                    const Twine &Name,
                                    BasicBlock *InsertAtEnd) {
assert(S->getType()->isPtrOrPtrVectorTy() && "Invalid cast");
assert((Ty->isIntOrIntVectorTy() || Ty->isPtrOrPtrVectorTy()) &&
       "Invalid cast");
assert(Ty->isVectorTy() == S->getType()->isVectorTy() && "Invalid cast");
assert((!Ty->isVectorTy() ||
        Ty->getVectorNumElements() == S->getType()->getVectorNumElements()) &&
       "Invalid cast");

if (Ty->isIntOrIntVectorTy())
  return Create(Instruction::PtrToInt, S, Ty, Name, InsertAtEnd);

/* ==== Call Your New method instead ==== */

Type *STy = S->getType();
if (STy->getPointerAddressSpace() != Ty->getPointerAddressSpace())
  return Create(Instruction::AddrSpaceCast, S, Ty, Name, InsertAtEnd);

return Create(Instruction::BitCast, S, Ty, Name, InsertAtEnd);

/* ====== */
}

Same thing for the instruction version.

Moreover, we should do the same for Constant *ConstantExpr::getPointerCast(Constant *S, Type *Ty).

Again maybe I am missing something, but at first glance, it seems at those points in the code path of XXXPointerCast we have the same restriction, isn’t it?

Thanks,
-Quentin

arsenm updated this revision to Diff 10767.Jun 23 2014, 2:47 PM

Make CreatePointerCast use CreatePointerBitCastOrAddrSpaceCast

arsenm accepted this revision.Jul 14 2014, 10:33 AM
arsenm added a reviewer: arsenm.

r212962

This revision is now accepted and ready to land.Jul 14 2014, 10:33 AM
arsenm closed this revision.Jul 14 2014, 10:33 AM