This is an archive of the discontinued LLVM Phabricator instance.

Differential D128320

AMDGPU: Don't crash on global_ctor/dtor declaration
ClosedPublic

Authored by ruiling on Jun 21 2022, 9:55 PM.

Details

Reviewers
arsenm
bcahoon
yaxunl
Commits
rG49b8ca3f7c4f: AMDGPU: Don't crash on global_ctor/dtor declaration

Diff Detail

Unit TestsFailed

TimeTest
60,140 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::scariness_score_test.cpp
60,110 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::scariness_score_test.cpp

Event Timeline

ruiling created this revision.Jun 21 2022, 9:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 9:55 PM
Herald added subscribers: kosarev, jsilvanus, foad and 8 others. · View Herald Transcript
ruiling requested review of this revision.Jun 21 2022, 9:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 9:55 PM
Herald added subscribers: llvm-commits, wdng. · View Herald Transcript
Harbormaster completed remote builds in B171241: Diff 438907.Jun 21 2022, 10:50 PM
foad added inline comments.Jun 22 2022, 12:04 AM
llvm/lib/Target/AMDGPU/AMDGPUCtorDtorLowering.cpp
56

Don't need the !GA check here any more.

Alternatively you could use dyn_cast_or_null on line 55.

nhaehnle added inline comments.Jun 22 2022, 3:17 AM
llvm/lib/Target/AMDGPU/AMDGPUCtorDtorLowering.cpp
56

Isn't this just being defensive about what type the initializer is? That seems wise to me, you also wouldn't need a dyn_cast otherwise.

foad added inline comments.Jun 22 2022, 3:27 AM
llvm/lib/Target/AMDGPU/AMDGPUCtorDtorLowering.cpp
56

You're right, please ignore all my comments :(

arsenm added inline comments.Jun 22 2022, 6:26 AM
llvm/test/CodeGen/AMDGPU/lower-ctor-dtor-declaration.ll
1

Don't need -filetype=asm. Also don't need -o - if you're doing redirected input

ruiling updated this revision to Diff 439227.Jun 22 2022, 6:03 PM

address review comments

arsenm accepted this revision.Jun 22 2022, 6:09 PM
This revision is now accepted and ready to land.Jun 22 2022, 6:09 PM
Harbormaster completed remote builds in B171479: Diff 439227.Jun 22 2022, 6:40 PM
This revision was landed with ongoing or failed builds.Jun 23 2022, 6:05 AM
Closed by commit rG49b8ca3f7c4f: AMDGPU: Don't crash on global_ctor/dtor declaration (authored by ruiling). · Explain Why
This revision was automatically updated to reflect the committed changes.
ruiling added a commit: rG49b8ca3f7c4f: AMDGPU: Don't crash on global_ctor/dtor declaration.

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
2 lines
test/
CodeGen/
AMDGPU/
8 lines

Diff 438907

llvm/lib/Target/AMDGPU/AMDGPUCtorDtorLowering.cpp

Show First 20 Lines • Show All 44 Lines • ▼ Show 20 Lines Function *createInitOrFiniKernelFunction(Module &M, bool IsCtor) {
if (IsCtor) if (IsCtor)
InitOrFiniKernel->addFnAttr("device-init"); InitOrFiniKernel->addFnAttr("device-init");
else else
InitOrFiniKernel->addFnAttr("device-fini"); InitOrFiniKernel->addFnAttr("device-fini");
return InitOrFiniKernel; return InitOrFiniKernel;
} }
bool createInitOrFiniKernel(Module &M, GlobalVariable *GV, bool IsCtor) { bool createInitOrFiniKernel(Module &M, GlobalVariable *GV, bool IsCtor) {
if (!GV) if (!GV || !GV->hasInitializer())
return false; return false;
ConstantArray *GA = dyn_cast<ConstantArray>(GV->getInitializer()); ConstantArray *GA = dyn_cast<ConstantArray>(GV->getInitializer());
if (!GA || GA->getNumOperands() == 0) if (!GA || GA->getNumOperands() == 0)
foadUnsubmitted
Not Done
ReplyInline Actions

Don't need the !GA check here any more.

Alternatively you could use dyn_cast_or_null on line 55.

foad: Don't need the `!GA` check here any more. Alternatively you could use dyn_cast_or_null on line…
nhaehnleUnsubmitted
Not Done
ReplyInline Actions

Isn't this just being defensive about what type the initializer is? That seems wise to me, you also wouldn't need a dyn_cast otherwise.

nhaehnle: Isn't this just being defensive about what type the initializer is? That seems wise to me, you…
foadUnsubmitted
Not Done
ReplyInline Actions

You're right, please ignore all my comments :(

foad: You're right, please ignore all my comments :(
return false; return false;
Function *InitOrFiniKernel = createInitOrFiniKernelFunction(M, IsCtor); Function *InitOrFiniKernel = createInitOrFiniKernelFunction(M, IsCtor);
IRBuilder<> IRB(InitOrFiniKernel->getEntryBlock().getTerminator()); IRBuilder<> IRB(InitOrFiniKernel->getEntryBlock().getTerminator());
for (Value *V : GA->operands()) { for (Value *V : GA->operands()) {
auto *CS = cast<ConstantStruct>(V); auto *CS = cast<ConstantStruct>(V);
if (Function *F = dyn_cast<Function>(CS->getOperand(1))) { if (Function *F = dyn_cast<Function>(CS->getOperand(1))) {
FunctionCallee Ctor = FunctionCallee Ctor =
M.getOrInsertFunction(F->getName(), IRB.getVoidTy()); M.getOrInsertFunction(F->getName(), IRB.getVoidTy());
Show All 31 Lines

llvm/test/CodeGen/AMDGPU/lower-ctor-dtor-declaration.ll

  • This file was added.
; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx700 -filetype=asm -o - < %s | FileCheck %s
arsenmUnsubmitted
Not Done
ReplyInline Actions

Don't need -filetype=asm. Also don't need -o - if you're doing redirected input

arsenm: Don't need -filetype=asm. Also don't need -o - if you're doing redirected input
@llvm.global_ctors = external global [2 x { i32, void ()*, i8* }]
@llvm.global_dtors = external global [2 x { i32, void ()*, i8* }]
; No amdgpu_kernels emitted for global_ctors declaration
; CHECK-NOT: amdgcn.device.init
; CHECK-NOT: amdgcn.device.fini