From da0c1904a827d900d351b706397e26864d52392c Mon Sep 17 00:00:00 2001 From: Tim Besard Date: Wed, 17 Apr 2024 15:03:10 +0200 Subject: [PATCH 1/3] Use getPointerAddressSpace instead of cast.getAddressSpace. The former also handles vectors of pointers, which can occur after vectorization: ``` #5 0x00007f5bfe94de5e in llvm::cast (Val=) at llvm/Support/Casting.h:578 578 assert(isa(Val) && "cast() 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(I.getDestTy())->getAddressSpace(); (rr) call I.dump() %23 = addrspacecast <4 x ptr addrspace(10)> %wide.load to <4 x ptr addrspace(11)>, !dbg !43 ``` --- src/cgutils.cpp | 6 +++--- src/llvm-alloc-opt.cpp | 5 +++-- src/llvm-gc-invariant-verifier.cpp | 19 ++++++++++--------- src/llvm-propagate-addrspaces.cpp | 2 +- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/cgutils.cpp b/src/cgutils.cpp index 8385b5682a0bc..344bc7c5be5b2 100644 --- a/src/cgutils.cpp +++ b/src/cgutils.cpp @@ -57,7 +57,7 @@ static Value *maybe_decay_untracked(jl_codectx_t &ctx, Value *V) static Value *decay_derived(jl_codectx_t &ctx, Value *V) { Type *T = V->getType(); - if (cast(T)->getAddressSpace() == AddressSpace::Derived) + if (T->getPointerAddressSpace() == AddressSpace::Derived) return V; // Once llvm deletes pointer element types, we won't need it here any more either. Type *NewT = PointerType::getWithSamePointeeType(cast(T), AddressSpace::Derived); @@ -68,7 +68,7 @@ static Value *decay_derived(jl_codectx_t &ctx, Value *V) static Value *maybe_decay_tracked(jl_codectx_t &ctx, Value *V) { Type *T = V->getType(); - if (cast(T)->getAddressSpace() != AddressSpace::Tracked) + if (T->getPointerAddressSpace() != AddressSpace::Tracked) return V; Type *NewT = PointerType::getWithSamePointeeType(cast(T), AddressSpace::Derived); return ctx.builder.CreateAddrSpaceCast(V, NewT); @@ -295,7 +295,7 @@ void jl_debugcache_t::initialize(Module *m) { static Value *emit_pointer_from_objref(jl_codectx_t &ctx, Value *V) { - unsigned AS = cast(V->getType())->getAddressSpace(); + unsigned AS = V->getType()->getPointerAddressSpace(); if (AS != AddressSpace::Tracked && AS != AddressSpace::Derived) return V; V = decay_derived(ctx, V); diff --git a/src/llvm-alloc-opt.cpp b/src/llvm-alloc-opt.cpp index 5df4f52aca425..08a884304747e 100644 --- a/src/llvm-alloc-opt.cpp +++ b/src/llvm-alloc-opt.cpp @@ -1067,10 +1067,11 @@ void Optimizer::splitOnStack(CallInst *orig_inst) store_ty = T_pjlvalue; } else { - store_ty = PointerType::getWithSamePointeeType(T_pjlvalue, cast(store_ty)->getAddressSpace()); + store_ty = PointerType::getWithSamePointeeType( + T_pjlvalue, store_ty->getPointerAddressSpace()); store_val = builder.CreateBitCast(store_val, store_ty); } - if (cast(store_ty)->getAddressSpace() != AddressSpace::Tracked) + if (store_ty->getPointerAddressSpace() != AddressSpace::Tracked) store_val = builder.CreateAddrSpaceCast(store_val, pass.T_prjlvalue); newstore = builder.CreateStore(store_val, slot.slot); } diff --git a/src/llvm-gc-invariant-verifier.cpp b/src/llvm-gc-invariant-verifier.cpp index 8c8e2e4546d04..6245cb0708423 100644 --- a/src/llvm-gc-invariant-verifier.cpp +++ b/src/llvm-gc-invariant-verifier.cpp @@ -62,8 +62,8 @@ struct GCInvariantVerifier : public InstVisitor { }; void GCInvariantVerifier::visitAddrSpaceCastInst(AddrSpaceCastInst &I) { - unsigned FromAS = cast(I.getSrcTy())->getAddressSpace(); - unsigned ToAS = cast(I.getDestTy())->getAddressSpace(); + unsigned FromAS = I.getSrcTy()->getPointerAddressSpace(); + unsigned ToAS = I.getDestTy()->getPointerAddressSpace(); if (FromAS == 0) return; Check(ToAS != AddressSpace::Loaded && FromAS != AddressSpace::Loaded, @@ -81,7 +81,7 @@ void GCInvariantVerifier::checkStoreInst(Type *VTy, unsigned AS, Value &SI) { if (VTy->isPointerTy()) { /* We currently don't obey this for arguments. That's ok - they're externally rooted. */ - unsigned AS = cast(VTy)->getAddressSpace(); + unsigned AS = VTy->getPointerAddressSpace(); Check(AS != AddressSpace::CalleeRooted && AS != AddressSpace::Derived, "Illegal store of decayed value", &SI); @@ -108,14 +108,14 @@ void GCInvariantVerifier::visitAtomicCmpXchgInst(AtomicCmpXchgInst &SI) { void GCInvariantVerifier::visitLoadInst(LoadInst &LI) { Type *Ty = LI.getType(); if (Ty->isPointerTy()) { - unsigned AS = cast(Ty)->getAddressSpace(); + unsigned AS = Ty->getPointerAddressSpace(); Check(AS != AddressSpace::CalleeRooted && AS != AddressSpace::Derived, "Illegal load of gc relevant value", &LI); } Ty = LI.getPointerOperand()->getType(); if (Ty->isPointerTy()) { - unsigned AS = cast(Ty)->getAddressSpace(); + unsigned AS = Ty->getPointerAddressSpace(); Check(AS != AddressSpace::CalleeRooted, "Illegal load of callee rooted value", &LI); } @@ -131,7 +131,7 @@ void GCInvariantVerifier::visitReturnInst(ReturnInst &RI) { Type *RTy = RI.getReturnValue()->getType(); if (!RTy->isPointerTy()) return; - unsigned AS = cast(RTy)->getAddressSpace(); + unsigned AS = RTy->getPointerAddressSpace(); Check(!isSpecialAS(AS) || AS == AddressSpace::Tracked, "Only gc tracked values may be directly returned", &RI); } @@ -140,7 +140,7 @@ void GCInvariantVerifier::visitGetElementPtrInst(GetElementPtrInst &GEP) { Type *Ty = GEP.getType(); if (!Ty->isPointerTy()) return; - unsigned AS = cast(Ty)->getAddressSpace(); + unsigned AS = Ty->getPointerAddressSpace(); if (!isSpecialAS(AS)) return; /* We're actually ok with GEPs here, as long as they don't feed into any @@ -170,8 +170,9 @@ void GCInvariantVerifier::visitCallInst(CallInst &CI) { continue; } Type *Ty = Arg->getType(); - Check(Ty->isPointerTy() && cast(Ty)->getAddressSpace() == AddressSpace::Tracked, - "Invalid derived pointer in jlcall", &CI); + Check(Ty->isPointerTy() && + Ty->getPointerAddressSpace() == AddressSpace::Tracked, + "Invalid derived pointer in jlcall", &CI); } } } diff --git a/src/llvm-propagate-addrspaces.cpp b/src/llvm-propagate-addrspaces.cpp index cc7dace28b24e..7c51fac5e1bd5 100644 --- a/src/llvm-propagate-addrspaces.cpp +++ b/src/llvm-propagate-addrspaces.cpp @@ -60,7 +60,7 @@ struct PropagateJuliaAddrspacesVisitor : public InstVisitor(V->getType())->getAddressSpace(); + return V->getType()->getPointerAddressSpace(); } static bool isSpecialAS(unsigned AS) { From 6acd36221d6f15dc2d7fac59f9f5162dbd78f54a Mon Sep 17 00:00:00 2001 From: Tim Besard Date: Wed, 17 Apr 2024 15:31:25 +0200 Subject: [PATCH 2/3] Add lit test. --- test/llvmpasses/gc-invariant-verifier.ll | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 test/llvmpasses/gc-invariant-verifier.ll diff --git a/test/llvmpasses/gc-invariant-verifier.ll b/test/llvmpasses/gc-invariant-verifier.ll new file mode 100644 index 0000000000000..ef32521427da1 --- /dev/null +++ b/test/llvmpasses/gc-invariant-verifier.ll @@ -0,0 +1,13 @@ +; This file is a part of Julia. License is MIT: https://julialang.org/license + +; RUN: opt -enable-new-pm=1 --opaque-pointers=1 --load-pass-plugin=libjulia-codegen%shlibext -passes='function(GCInvariantVerifier)' -S %s | FileCheck %s + +; CHECK-LABEL: @vectorized_addrspacecast +define ptr addrspace(10) @vectorized_addrspacecast() { +top: + ret ptr addrspace(10) null + +vector.ph: + %0 = addrspacecast <4 x ptr addrspace(10)> zeroinitializer to <4 x ptr addrspace(11)> + unreachable +} From 51e4492161bf89892eb645320c69e59f714d7ae3 Mon Sep 17 00:00:00 2001 From: Tim Besard Date: Thu, 18 Apr 2024 11:06:31 +0200 Subject: [PATCH 3/3] Extend some pointer type checks to cover vectors. --- src/llvm-gc-invariant-verifier.cpp | 12 ++++++------ src/llvm-propagate-addrspaces.cpp | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/llvm-gc-invariant-verifier.cpp b/src/llvm-gc-invariant-verifier.cpp index 6245cb0708423..5badbca807569 100644 --- a/src/llvm-gc-invariant-verifier.cpp +++ b/src/llvm-gc-invariant-verifier.cpp @@ -78,7 +78,7 @@ void GCInvariantVerifier::visitAddrSpaceCastInst(AddrSpaceCastInst &I) { } void GCInvariantVerifier::checkStoreInst(Type *VTy, unsigned AS, Value &SI) { - if (VTy->isPointerTy()) { + if (VTy->isPtrOrPtrVectorTy()) { /* We currently don't obey this for arguments. That's ok - they're externally rooted. */ unsigned AS = VTy->getPointerAddressSpace(); @@ -107,14 +107,14 @@ void GCInvariantVerifier::visitAtomicCmpXchgInst(AtomicCmpXchgInst &SI) { void GCInvariantVerifier::visitLoadInst(LoadInst &LI) { Type *Ty = LI.getType(); - if (Ty->isPointerTy()) { + if (Ty->isPtrOrPtrVectorTy()) { unsigned AS = Ty->getPointerAddressSpace(); Check(AS != AddressSpace::CalleeRooted && AS != AddressSpace::Derived, "Illegal load of gc relevant value", &LI); } Ty = LI.getPointerOperand()->getType(); - if (Ty->isPointerTy()) { + if (Ty->isPtrOrPtrVectorTy()) { unsigned AS = Ty->getPointerAddressSpace(); Check(AS != AddressSpace::CalleeRooted, "Illegal load of callee rooted value", &LI); @@ -129,7 +129,7 @@ void GCInvariantVerifier::visitReturnInst(ReturnInst &RI) { if (!RI.getReturnValue()) return; Type *RTy = RI.getReturnValue()->getType(); - if (!RTy->isPointerTy()) + if (!RTy->isPtrOrPtrVectorTy()) return; unsigned AS = RTy->getPointerAddressSpace(); Check(!isSpecialAS(AS) || AS == AddressSpace::Tracked, @@ -138,7 +138,7 @@ void GCInvariantVerifier::visitReturnInst(ReturnInst &RI) { void GCInvariantVerifier::visitGetElementPtrInst(GetElementPtrInst &GEP) { Type *Ty = GEP.getType(); - if (!Ty->isPointerTy()) + if (!Ty->isPtrOrPtrVectorTy()) return; unsigned AS = Ty->getPointerAddressSpace(); if (!isSpecialAS(AS)) @@ -170,7 +170,7 @@ void GCInvariantVerifier::visitCallInst(CallInst &CI) { continue; } Type *Ty = Arg->getType(); - Check(Ty->isPointerTy() && + Check(Ty->isPtrOrPtrVectorTy() && Ty->getPointerAddressSpace() == AddressSpace::Tracked, "Invalid derived pointer in jlcall", &CI); } diff --git a/src/llvm-propagate-addrspaces.cpp b/src/llvm-propagate-addrspaces.cpp index 7c51fac5e1bd5..485eabefb5f8b 100644 --- a/src/llvm-propagate-addrspaces.cpp +++ b/src/llvm-propagate-addrspaces.cpp @@ -139,7 +139,7 @@ Value *PropagateJuliaAddrspacesVisitor::LiftPointer(Module *M, Value *V, Instruc break; } else { // Ok, we've reached a leaf - check if it is eligible for lifting - if (!CurrentV->getType()->isPointerTy() || + if (!CurrentV->getType()->isPtrOrPtrVectorTy() || isSpecialAS(getValueAddrSpace(CurrentV))) { // If not, poison all (recursive) users of this value, to prevent // looking at them again in future iterations.