-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[WIP][IR][Constants] Change the semantic of ConstantPointerNull to represent an actual nullptr instead of a zero-value pointer
#166667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| // RUN: %clang_cc1 %s -O0 -triple amdgcn -emit-llvm -o - | FileCheck %s | ||
| // RUN: %clang_cc1 %s -O0 -triple amdgcn---opencl -emit-llvm -o - | FileCheck %s | ||
|
|
||
| // CHECK: target datalayout = "e-m:e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128:128:48-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7:8:9" | ||
| // CHECK: target datalayout = "e-m:e-p:64:64-p1:64:64-po2:32:32-po3:32:32-p4:64:64-po5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128:128:48-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7:8:9" | ||
| void foo(void) {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1497,6 +1497,21 @@ Constant *llvm::ConstantFoldCastOperand(unsigned Opcode, Constant *C, | |
| llvm_unreachable("Missing case"); | ||
| case Instruction::PtrToAddr: | ||
| case Instruction::PtrToInt: | ||
| // If the input is a nullptr, we can fold it to the corresponding nullptr | ||
| // value. | ||
| if (Opcode == Instruction::PtrToInt && C->isNullValue()) { | ||
| if (std::optional<APInt> NullPtrValue = DL.getNullPtrValue( | ||
| C->getType()->getScalarType()->getPointerAddressSpace())) { | ||
| if (NullPtrValue->isZero()) { | ||
| return Constant::getZeroValue(DestTy); | ||
| } else if (NullPtrValue->isAllOnes()) { | ||
| return ConstantInt::get( | ||
| DestTy, NullPtrValue->zextOrTrunc(DestTy->getScalarSizeInBits())); | ||
| } else { | ||
| llvm_unreachable("invalid nullptr value"); | ||
| } | ||
| } | ||
| } | ||
| if (auto *CE = dyn_cast<ConstantExpr>(C)) { | ||
| Constant *FoldedValue = nullptr; | ||
| // If the input is an inttoptr, eliminate the pair. This requires knowing | ||
|
|
@@ -1543,6 +1558,13 @@ Constant *llvm::ConstantFoldCastOperand(unsigned Opcode, Constant *C, | |
| } | ||
| break; | ||
| case Instruction::IntToPtr: | ||
| // We can fold it to a null pointer if the input is the nullptr value. | ||
| if (std::optional<APInt> NullPtrValue = DL.getNullPtrValue( | ||
| DestTy->getScalarType()->getPointerAddressSpace())) { | ||
| if ((NullPtrValue->isZero() && C->isZeroValue()) || | ||
| (NullPtrValue->isAllOnes() && C->isAllOnesValue())) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we have to check the width of the input value for all ones since inttoptr zero-extends, so a short value would not end up as canonical nullptr. |
||
| return Constant::getNullValue(DestTy); | ||
arichardson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| // If the input is a ptrtoint, turn the pair into a ptr to ptr bitcast if | ||
| // the int size is >= the ptr size and the address spaces are the same. | ||
| // This requires knowing the width of a pointer, so it can't be done in | ||
|
|
@@ -1561,6 +1583,24 @@ Constant *llvm::ConstantFoldCastOperand(unsigned Opcode, Constant *C, | |
| } | ||
| } | ||
| break; | ||
| case Instruction::AddrSpaceCast: | ||
| // A null pointer (`ptr addrspace(N) null` in IR presentation, | ||
| // `ConstantPointerNull` in LLVM class, not `nullptr` in C/C++) used to | ||
| // represent a zero-value pointer in the corresponding address space. | ||
| // Therefore, we can't simply fold an address space cast of a null pointer | ||
| // from one address space to another, because on some targets, the nullptr | ||
| // of an address space could be non-zero. | ||
| // | ||
| // Recently, the semantic of `ptr addrspace(N) null` is changed to represent | ||
| // the actual nullptr in the corresponding address space. It can be zero or | ||
| // non-zero, depending on the target. Therefore, we can fold an address | ||
| // space cast of a nullptr from one address space to another. | ||
|
|
||
| // If the input is a nullptr, we can fold it to the corresponding | ||
| // nullptr in the destination address space. | ||
| if (C->isNullValue()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this actually true? Are null values in one address space always null in all others? I imagine this is almost always true, but maybe you could have some where this conversion is not valid?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see this property in langref so we should either add it or drop this fold.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is actually the whole point of this PR. The semantic would be, convert the I'll clarify this in the LangRef.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good |
||
| return Constant::getNullValue(DestTy); | ||
| [[fallthrough]]; | ||
| case Instruction::Trunc: | ||
| case Instruction::ZExt: | ||
| case Instruction::SExt: | ||
|
|
@@ -1570,7 +1610,6 @@ Constant *llvm::ConstantFoldCastOperand(unsigned Opcode, Constant *C, | |
| case Instruction::SIToFP: | ||
| case Instruction::FPToUI: | ||
| case Instruction::FPToSI: | ||
| case Instruction::AddrSpaceCast: | ||
| break; | ||
| case Instruction::BitCast: | ||
| return FoldBitCast(C, DestTy, DL); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4275,8 +4275,15 @@ static void emitGlobalConstantImpl(const DataLayout &DL, const Constant *CV, | |||||||||||||||||||||
| return emitGlobalConstantFP(CFP, AP); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (isa<ConstantPointerNull>(CV)) { | ||||||||||||||||||||||
| AP.OutStreamer->emitIntValue(0, Size); | ||||||||||||||||||||||
| if (auto *NullPtr = dyn_cast<ConstantPointerNull>(CV)) { | ||||||||||||||||||||||
| if (std::optional<APInt> NullPtrVal = | ||||||||||||||||||||||
| DL.getNullPtrValue(NullPtr->getType()->getPointerAddressSpace())) { | ||||||||||||||||||||||
| AP.OutStreamer->emitIntValue(NullPtrVal->getSExtValue(), Size); | ||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| // We fall back to the default behavior of emitting a zero value if we | ||||||||||||||||||||||
| // can't get the null pointer value from the data layout. | ||||||||||||||||||||||
| AP.OutStreamer->emitIntValue(0, Size); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+4279
to
+4286
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I wonder if we should drop the std::optional from the DataLayout and just always have a defined value? We could then also either drop the 'c' flag or do something like Silently falling back to zero here seems like it would cause more pain for hypothetical targets with custom null pointers than forcing them to set the datalayout?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds like a good idea. I can do that but I guess we'd need an RFC. |
||||||||||||||||||||||
| return; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is equivalent and a bit simpler.