Page MenuHomePhabricator

Fix assert with copy from global through addrspacecast
ClosedPublic

Authored by arsenm on Nov 17 2013, 4:29 PM.

Details

Reviewers
michele.scandale

Diff Detail

Event Timeline

lib/IR/Constants.cpp
1510

Maybe this function can be fused within getPointerCast in order to support there the case of pointers with different address spaces.

arsenm added inline comments.Nov 22 2013, 12:13 PM
lib/IR/Constants.cpp
1510

I tried that originally, but the results weren't satisfying. The asserts are different since getBitCastOrAddrSpaceCast only accepts pointer<->pointer casts, while getPointerCast accepts integer destinations.

lib/IR/Constants.cpp
1510

That's ok, IMHO a more general name would be better (like getPointerToPointerCast).

However, the getPointerCast must be fixed in order to create the proper cast also when the destination type has a different address space with respect to the source Constant.

arsenm added inline comments.Dec 6 2013, 12:51 PM
lib/IR/Constants.cpp
1510

That is already handled, I just updated the comment

lib/IR/Constants.cpp
1510

I think that the semantic for 'getBitCastOrAddrSpaceCast' should be to pick a bitcast wheter is valid or an addrspace cast whether is valid or assert.
A set of functions like this, also for instructions, and a wrapper in the IRBuilder will be very useful to simplify the upgrade of the frontend and other tools to the new bitcast semantic and the new instruction (e.g. where previously there was a CreateBitCast now a CreateBitCastOrAddrSpaceCast would be fine).

What do you think about this?

arsenm added a comment.Dec 6 2013, 3:55 PM

I think it's better for it to be stricter and only work for pointers. A better name might be getPointerBitCastOrAddrSpaceCast? IRBuilder equivalents of whatever is there is a good idea.

The suggested name won't be ambiguous, so +1.

BTW, I don't see the problem, if a non pointer value is passed, an addrspacecast would be illegal because it requires a pointer value, while a bitcast would be valid w.r.t. its constraints.

Constant *ConstantExpr::getBitCastOrAddrSpaceCast(Constant *S, Type *Ty) {       
  assert((CastInst::castIsValid(Instruction::BitCast, S, Ty) ||                  
          CastInst::castIsValid(Instruction::AddrSpaceCast, S, Ty)) &&           
          "Invalid cast");                                                       
                                                                               
  unsigned SrcTy = S->getType();                                                 
  if (Ty->isPtrOrPtrVectorTy() && DestTy->isPtrOrPtrVectorTy() &&                
      SrcTy->getPointerAddressSpace() != Ty->getPointerAddressSpace())           
    return getAddrSpaceCast(S, Ty);                                              
                                                                               
  return getBitCast(S, Ty);                                                      
}

But maybe something like this could just an helper in IRBuilder...

arsenm added a comment.Dec 6 2013, 4:27 PM

The problem is passing in something that isn't a pointer that you didn't mean to. Generally doing whatever it takes to convert types isn't desirable. If you're expecting to get a pointer-only cast like addrspacecast, you probably don't want an integer to be passed in.

arsenm added a comment.Dec 6 2013, 4:33 PM

Also, not accepting integers is also the distinguishing feature from getPointerCast

That's fine, with the name 'getPointerBitCastOrAddrSpaceCast' it will avoid ambiguities.

arsenm closed this revision.Dec 6 2013, 7:04 PM

r196638