Skip to content
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

Use getPointerAddressSpace instead of cast<PointerType>.getAddressSpace. #54113

Merged
merged 3 commits into from Apr 19, 2024

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Apr 17, 2024

The former also handles vectors of pointers, which can occur after vectorization:

#5  0x00007f5bfe94de5e in llvm::cast<llvm::PointerType, llvm::Type> (Val=<optimized out>) at llvm/Support/Casting.h:578
578	  assert(isa<To>(Val) && "cast<Ty>() argument of incompatible type!");

(rr) up
#6  GCInvariantVerifier::visitAddrSpaceCastInst (this=this@entry=0x7ffd022fbf56, I=...) at julia/src/llvm-gc-invariant-verifier.cpp:66
66	    unsigned ToAS = cast<PointerType>(I.getDestTy())->getAddressSpace();

(rr) call I.dump()
%23 = addrspacecast <4 x ptr addrspace(10)> %wide.load to <4 x ptr addrspace(11)>, !dbg !43

Fixes aborts seen in #53070; cc @gbaraldi

@maleadt maleadt added the llvm For issues that relate to LLVM label Apr 17, 2024
@maleadt maleadt mentioned this pull request Apr 17, 2024
@giordano
Copy link
Contributor

Thanks for looking into this! Is there any simple test we can add to catch similar issue early, without having to run nanosoldier?

@maleadt
Copy link
Member Author

maleadt commented Apr 17, 2024

Sure. I can add a lit test.

Copy link
Sponsor Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any guard that is Ty->isPointerType probably needs to be changed to isPtrOrPtrVectorTy otherwise some of these changes will ignore PtrVector

@maleadt
Copy link
Member Author

maleadt commented Apr 18, 2024

Any guard that is Ty->isPointerType probably needs to be changed to isPtrOrPtrVectorTy otherwise some of these changes will ignore PtrVector

I don't think all of them need to be changed. Yes, in the GC invariant verifier (and I'll push that shortly), but most of the code in codegen/cgutils doesn't actually know how to handle vectors of pointers, or expects to encounter them pre-optimization anyway. The change to getPointerAddressSpace probably isn't very useful in there, but I don't think it can hurt either (since subsequent cast<PointerType>s are expected to abort anyway).

The former also handles vectors of pointers, which can occur after vectorization:

```
#5  0x00007f5bfe94de5e in llvm::cast<llvm::PointerType, llvm::Type> (Val=<optimized out>) at llvm/Support/Casting.h:578
578	  assert(isa<To>(Val) && "cast<Ty>() argument of incompatible type!");

(rr) up
#6  GCInvariantVerifier::visitAddrSpaceCastInst (this=this@entry=0x7ffd022fbf56, I=...) at julia/src/llvm-gc-invariant-verifier.cpp:66
66	    unsigned ToAS = cast<PointerType>(I.getDestTy())->getAddressSpace();

(rr) call I.dump()
%23 = addrspacecast <4 x ptr addrspace(10)> %wide.load to <4 x ptr addrspace(11)>, !dbg !43
```
@giordano giordano merged commit 074ea2a into master Apr 19, 2024
7 checks passed
@giordano giordano deleted the tb/getpointeraddrspace branch April 19, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm For issues that relate to LLVM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants