[Attributor] Avoid AS-cast for function pointers #65825
Conversation
Based on the post-commit discussion of llvm@3764271
| if (FP->getType()->getPointerAddressSpace()) | ||
| FP = new AddrSpaceCastInst(FP, PointerType::get(FP->getType(), 0), | ||
| FP->getName() + ".as0", CB); | ||
| unsigned ProgramAS = A.getDataLayout().getProgramAddressSpace(); |
There was a problem hiding this comment.
The call has an address space and the underlying function has an address space. You aren't creating a new function so you don't need to query the datalayout
There was a problem hiding this comment.
At this point I need to know what the AS is of the functions I compare with (in the if-cascade). I could pick one of them and use its AS, or I use the DL. This seems cleaner.
There was a problem hiding this comment.
This is wrong as soon as you mix address spaces. Ideally would check they all are the same and use whatever it is.
There was a problem hiding this comment.
This is IMO more correct that the previous hardcoded AS0, but since I am not familiar with the optimization I will defer to @arsenm
| FP = new AddrSpaceCastInst(FP, PointerType::get(FP->getType(), 0), | ||
| FP->getName() + ".as0", CB); | ||
| } else { | ||
| FP = new IntToPtrInst( |
There was a problem hiding this comment.
One thing here is that we can't safely introduce this for non-integral pointers, and probably do want the addrspacecast in that case?
There was a problem hiding this comment.
If the AS cast is not a no-op, and this is not legal, we likely want to do nothing. How can I check for non-integral pointers to skip the generation of the if-cascade?
Also, are non-integral function pointers a thing?
There was a problem hiding this comment.
Also, are non-integral function pointers a thing?
Nonintegralness is a property of the address space only, what it's pointing at doesn't matter. You check non-integralness with DL.isNonIntegralAddressSpace, but I don't se why you would want it here
There was a problem hiding this comment.
The restriction to not allow inttoptr for nonintegral address spaces was relaxed a long time ago
| unsigned PtrAS = FP->getType()->getPointerAddressSpace(); | ||
| // TODO: All we really want is a bitcast, see: | ||
| // https://github.com/llvm/llvm-project/commit/37642714edfc382be90ab8ec091e0261465c1b47#r126273142 | ||
| if (PtrAS != ProgramAS) { |
There was a problem hiding this comment.
| FP = new AddrSpaceCastInst(FP, PointerType::get(FP->getType(), 0), | ||
| FP->getName() + ".as0", CB); | ||
| } else { | ||
| FP = new IntToPtrInst( |
There was a problem hiding this comment.
The restriction to not allow inttoptr for nonintegral address spaces was relaxed a long time ago
| if (FP->getType()->getPointerAddressSpace()) | ||
| FP = new AddrSpaceCastInst(FP, PointerType::get(FP->getType(), 0), | ||
| FP->getName() + ".as0", CB); | ||
| unsigned ProgramAS = A.getDataLayout().getProgramAddressSpace(); |
There was a problem hiding this comment.
This is wrong as soon as you mix address spaces. Ideally would check they all are the same and use whatever it is.
| // TODO: All we really want is a bitcast, see: | ||
| // https://github.com/llvm/llvm-project/commit/37642714edfc382be90ab8ec091e0261465c1b47#r126273142 | ||
| if (PtrAS != ProgramAS) { | ||
| if (TTI->isNoopAddrSpaceCast(PtrAS, ProgramAS)) { |
There was a problem hiding this comment.
I don't see why isNoopAddrSpaceCast matters here. You're just casting to perform the call, so any TTI::isValidAddrSpaceCast would do? The fallback to introduce inttoptr doesn't make sense
Based on the post-commit discussion of
3764271
we now avoid the AS cast if it is not a no-op.