From bfaa8f0f17212045a272b7339c960c9a5bdc23d4 Mon Sep 17 00:00:00 2001 From: tnowicki Date: Tue, 21 Jan 2025 09:20:44 -0500 Subject: [PATCH 1/6] Allow Visitor to support CallInst * We have encountered IR where it is necessary to transform a CallInst. * This requires a CallInst visitor. Although, a CallInst visitor can be added to the OpMap, it fails to find the CallInst visitor. Instead it gives up when it determines the Call is not an intrinsic or llvm-dialect op. * This change ensures we can find the CallInst visitor. --- include/llvm-dialects/Dialect/OpMap.h | 28 +++++++++++---------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/include/llvm-dialects/Dialect/OpMap.h b/include/llvm-dialects/Dialect/OpMap.h index 1d18bfd..355d15d 100644 --- a/include/llvm-dialects/Dialect/OpMap.h +++ b/include/llvm-dialects/Dialect/OpMap.h @@ -595,13 +595,14 @@ template class OpMapIteratorBase final { if (std::get(m_iterator) == map->m_intrinsics.end()) invalidate(); } - } else { - createFromDialectOp(desc.getMnemonic()); + } else if (!createFromDialectOp(desc.getMnemonic())) { + invalidate(); } } OpMapIteratorBase(OpMapT *map, const llvm::Function &func) : m_map{map} { - createFromFunc(func); + if (!createFromFunc(func)) + invalidate(); } // Do a lookup for a given instruction. Mark the iterator as invalid @@ -610,19 +611,13 @@ template class OpMapIteratorBase final { if (auto *CI = llvm::dyn_cast(&inst)) { const llvm::Function *callee = CI->getCalledFunction(); if (callee) { - createFromFunc(*callee); - return; + if (createFromFunc(*callee)) + return; } } const unsigned op = inst.getOpcode(); - // Construct an invalid iterator. - if (op == llvm::Instruction::Call || op == llvm::Instruction::CallBr) { - invalidate(); - return; - } - BaseIteratorT it = m_map->m_coreOpcodes.find(op); if (it != m_map->m_coreOpcodes.end()) { m_desc = OpDescription::fromCoreOp(op); @@ -699,20 +694,20 @@ template class OpMapIteratorBase final { private: void invalidate() { m_isInvalid = true; } - void createFromFunc(const llvm::Function &func) { + bool createFromFunc(const llvm::Function &func) { if (func.isIntrinsic()) { m_iterator = m_map->m_intrinsics.find(func.getIntrinsicID()); if (std::get(m_iterator) != m_map->m_intrinsics.end()) { m_desc = OpDescription::fromIntrinsic(func.getIntrinsicID()); - return; + return true; } } - createFromDialectOp(func.getName()); + return createFromDialectOp(func.getName()); } - void createFromDialectOp(llvm::StringRef funcName) { + bool createFromDialectOp(llvm::StringRef funcName) { size_t idx = 0; bool found = false; for (auto &dialectOpKV : m_map->m_dialectOps) { @@ -729,8 +724,7 @@ template class OpMapIteratorBase final { ++idx; } - if (!found) - invalidate(); + return found; } // Re-construct base OpDescription from the stored iterator. From 7223fdfa80c971f66a0ead4699e092e3f6d7e1f9 Mon Sep 17 00:00:00 2001 From: tnowicki Date: Tue, 21 Jan 2025 12:27:20 -0500 Subject: [PATCH 2/6] Tests for looking up an Instruction::Call in an OpMap with a CallInst --- test/unit/interface/OpMapIRTests.cpp | 47 ++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/test/unit/interface/OpMapIRTests.cpp b/test/unit/interface/OpMapIRTests.cpp index a0aa6c8..b0d9551 100644 --- a/test/unit/interface/OpMapIRTests.cpp +++ b/test/unit/interface/OpMapIRTests.cpp @@ -118,12 +118,13 @@ TEST_F(OpMapIRTestFixture, IntrinsicOpMatchesInstructionTest) { EXPECT_EQ(map[AssumeDesc], "assume"); const auto &SideEffect = *B.CreateCall( - Intrinsic::getDeclaration(Mod.get(), Intrinsic::sideeffect)); + Intrinsic::getOrInsertDeclaration(Mod.get(), Intrinsic::sideeffect)); const std::array AssumeArgs = { ConstantInt::getBool(Type::getInt1Ty(Context), true)}; const auto &Assume = *B.CreateCall( - Intrinsic::getDeclaration(Mod.get(), Intrinsic::assume), AssumeArgs); + Intrinsic::getOrInsertDeclaration(Mod.get(), Intrinsic::assume), + AssumeArgs); EXPECT_FALSE(map.lookup(SideEffect) == map.lookup(Assume)); EXPECT_EQ(map.lookup(SideEffect), "sideeffect"); @@ -171,7 +172,7 @@ TEST_F(OpMapIRTestFixture, MixedOpMatchesInstructionTest) { EXPECT_EQ(map[SideEffectDesc], "sideeffect"); const auto &SideEffect = *B.CreateCall( - Intrinsic::getDeclaration(Mod.get(), Intrinsic::sideeffect)); + Intrinsic::getOrInsertDeclaration(Mod.get(), Intrinsic::sideeffect)); EXPECT_EQ(map.lookup(SideEffect), "sideeffect"); @@ -252,3 +253,43 @@ TEST_F(OpMapIRTestFixture, DialectOpOverloadTests) { EXPECT_EQ(map.lookup(Op1), "DialectOp4"); EXPECT_EQ(map.lookup(Op2), "DialectOp4"); } + +TEST_F(OpMapIRTestFixture, CallInstCoreOpMatchesInstructionTest) { + // Declare %OpaqueTy = type opaque + StructType *OpaqueTy = StructType::create(Context, "OpaqueTy"); + + // Define types + PointerType *OpaquePtrTy = PointerType::get(OpaqueTy, 0); + IntegerType *I32Ty = Type::getInt32Ty(Context); + + // Declare: %OpaqueTy* @ProcOpaqueHandle(i32, %OpaqueTy*) + FunctionType *ProcOpaqueHandleFuncTyTy = + FunctionType::get(OpaquePtrTy, {I32Ty, OpaquePtrTy}, false); + FunctionCallee ProcOpaqueHandleFuncTy = + Mod->getOrInsertFunction("ProcOpaqueHandle", ProcOpaqueHandleFuncTyTy); + + IRBuilder<> Builder{Context}; + Builder.SetInsertPoint(getEntryBlock()); + + // Create a dummy global variable of type %OpaqueTy* + GlobalVariable *GV = new GlobalVariable( + *Mod, OpaqueTy, false, GlobalValue::PrivateLinkage, nullptr, "handle"); + GV->setInitializer(ConstantAggregateZero::get(OpaqueTy)); + Value *Op2 = GV; + + // Create a constant value (e.g., 123) + Value *Op1 = ConstantInt::get(I32Ty, 123); + + // Build the call to ProcOpaqueHandle + Value *Args[] = {Op1, Op2}; + const CallInst &CallInst = *Builder.CreateCall(ProcOpaqueHandleFuncTy, Args); + + // Add Instruction::Call to OpMap + OpMap map; + const OpDescription CallDesc = OpDescription::fromCoreOp(Instruction::Call); + map[CallDesc] = "ProcOpaqueHandle"; + + // Look up the CallInst in the map and verify it finds the entry for + // Instruction::Call + EXPECT_EQ(map.lookup(CallInst), "ProcOpaqueHandle"); +} From 7865204dc0565f2bdbcd9ed42f4ffc5d5d299ce6 Mon Sep 17 00:00:00 2001 From: tnowicki Date: Wed, 22 Jan 2025 11:38:48 -0500 Subject: [PATCH 3/6] Updated with reviewer feedback --- include/llvm-dialects/Dialect/OpMap.h | 9 ++-- test/unit/interface/OpMapIRTests.cpp | 60 ++++++++++++++++++--------- 2 files changed, 44 insertions(+), 25 deletions(-) diff --git a/include/llvm-dialects/Dialect/OpMap.h b/include/llvm-dialects/Dialect/OpMap.h index 355d15d..5873fea 100644 --- a/include/llvm-dialects/Dialect/OpMap.h +++ b/include/llvm-dialects/Dialect/OpMap.h @@ -605,15 +605,12 @@ template class OpMapIteratorBase final { invalidate(); } - // Do a lookup for a given instruction. Mark the iterator as invalid - // if the instruction is a call-like core instruction. + // Do a lookup for a given instruction. OpMapIteratorBase(OpMapT *map, const llvm::Instruction &inst) : m_map{map} { if (auto *CI = llvm::dyn_cast(&inst)) { const llvm::Function *callee = CI->getCalledFunction(); - if (callee) { - if (createFromFunc(*callee)) - return; - } + if (callee && createFromFunc(*callee)) + return; } const unsigned op = inst.getOpcode(); diff --git a/test/unit/interface/OpMapIRTests.cpp b/test/unit/interface/OpMapIRTests.cpp index b0d9551..59ec0fd 100644 --- a/test/unit/interface/OpMapIRTests.cpp +++ b/test/unit/interface/OpMapIRTests.cpp @@ -254,22 +254,24 @@ TEST_F(OpMapIRTestFixture, DialectOpOverloadTests) { EXPECT_EQ(map.lookup(Op2), "DialectOp4"); } -TEST_F(OpMapIRTestFixture, CallInstCoreOpMatchesInstructionTest) { - // Declare %OpaqueTy = type opaque - StructType *OpaqueTy = StructType::create(Context, "OpaqueTy"); +TEST_F(OpMapIRTestFixture, CallCoreOpMatchesInstructionTest) { + OpMap map; + llvm_dialects::Builder B{Context}; // Define types - PointerType *OpaquePtrTy = PointerType::get(OpaqueTy, 0); + PointerType *PtrTy = B.getPtrTy(); IntegerType *I32Ty = Type::getInt32Ty(Context); - // Declare: %OpaqueTy* @ProcOpaqueHandle(i32, %OpaqueTy*) - FunctionType *ProcOpaqueHandleFuncTyTy = - FunctionType::get(OpaquePtrTy, {I32Ty, OpaquePtrTy}, false); - FunctionCallee ProcOpaqueHandleFuncTy = - Mod->getOrInsertFunction("ProcOpaqueHandle", ProcOpaqueHandleFuncTyTy); + // Declare: %ptr @ProcOpaqueHandle(i32, %ptr) + FunctionType *ProcOpaqueHandleFuncTy = + FunctionType::get(PtrTy, {I32Ty, PtrTy}, false); + FunctionCallee ProcOpaqueHandleFunc = + Mod->getOrInsertFunction("ProcOpaqueHandle", ProcOpaqueHandleFuncTy); - IRBuilder<> Builder{Context}; - Builder.SetInsertPoint(getEntryBlock()); + B.SetInsertPoint(getEntryBlock()); + + // Declare %OpaqueTy = type opaque + StructType *OpaqueTy = StructType::create(Context, "OpaqueTy"); // Create a dummy global variable of type %OpaqueTy* GlobalVariable *GV = new GlobalVariable( @@ -278,18 +280,38 @@ TEST_F(OpMapIRTestFixture, CallInstCoreOpMatchesInstructionTest) { Value *Op2 = GV; // Create a constant value (e.g., 123) - Value *Op1 = ConstantInt::get(I32Ty, 123); + Value *Op1 = B.getInt32(123); - // Build the call to ProcOpaqueHandle + // Build a call instruction Value *Args[] = {Op1, Op2}; - const CallInst &CallInst = *Builder.CreateCall(ProcOpaqueHandleFuncTy, Args); + const CallInst &Call = *B.CreateCall(ProcOpaqueHandleFunc, Args); + + // Create basic blocks for the function + auto *FC = getEntryBlock()->getParent(); + BasicBlock *Label1BB = BasicBlock::Create(Context, "label1", FC); + BasicBlock *Label2BB = BasicBlock::Create(Context, "label2", FC); + BasicBlock *ContinueBB = BasicBlock::Create(Context, "continue", FC); + + // Simulate a function that can branch to multiple labels + // For demonstration purposes, we'll create a placeholder function that represents this behavior + FunctionType *BranchFuncTy = FunctionType::get(Type::getVoidTy(Context), false); + FunctionCallee BranchFunc = Mod->getOrInsertFunction("Branch", BranchFuncTy); + + // Create the CallBr instruction + const CallBrInst &CallBr = *B.CreateCallBr(BranchFunc, ContinueBB, {Label1BB, Label2BB}); + + // Load and test OpMap with Call and CallBr // Add Instruction::Call to OpMap - OpMap map; const OpDescription CallDesc = OpDescription::fromCoreOp(Instruction::Call); - map[CallDesc] = "ProcOpaqueHandle"; + map[CallDesc] = "Call"; + + // Add Instruction::CallBr to OpMap + const OpDescription CallBrDesc = OpDescription::fromCoreOp(Instruction::CallBr); + map[CallBrDesc] = "CallBr"; - // Look up the CallInst in the map and verify it finds the entry for - // Instruction::Call - EXPECT_EQ(map.lookup(CallInst), "ProcOpaqueHandle"); + // Look up the Call and CallBr in the map and verify it finds the entries for + // Instruction::Call and Instruction::CallBr + EXPECT_EQ(map.lookup(Call), "Call"); + EXPECT_EQ(map.lookup(CallBr), "CallBr"); } From 34f8b85d54d3f69d7b45e018d876914caaa10ac7 Mon Sep 17 00:00:00 2001 From: Tyler Nowicki Date: Wed, 22 Jan 2025 12:22:00 -0500 Subject: [PATCH 4/6] Update test/unit/interface/OpMapIRTests.cpp Co-authored-by: Sebastian Neubauer --- test/unit/interface/OpMapIRTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/interface/OpMapIRTests.cpp b/test/unit/interface/OpMapIRTests.cpp index 59ec0fd..4895394 100644 --- a/test/unit/interface/OpMapIRTests.cpp +++ b/test/unit/interface/OpMapIRTests.cpp @@ -262,7 +262,7 @@ TEST_F(OpMapIRTestFixture, CallCoreOpMatchesInstructionTest) { PointerType *PtrTy = B.getPtrTy(); IntegerType *I32Ty = Type::getInt32Ty(Context); - // Declare: %ptr @ProcOpaqueHandle(i32, %ptr) + // Declare: ptr @ProcOpaqueHandle(i32, ptr) FunctionType *ProcOpaqueHandleFuncTy = FunctionType::get(PtrTy, {I32Ty, PtrTy}, false); FunctionCallee ProcOpaqueHandleFunc = From ed9cf9ae62a701b2c5d920522fc43f9fe3594be1 Mon Sep 17 00:00:00 2001 From: tnowicki Date: Wed, 22 Jan 2025 13:04:55 -0500 Subject: [PATCH 5/6] Added CallInst and CallBrInst to the Visitor Example --- example/ExampleMain.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/example/ExampleMain.cpp b/example/ExampleMain.cpp index d6371b9..18b4c5f 100644 --- a/example/ExampleMain.cpp +++ b/example/ExampleMain.cpp @@ -264,6 +264,12 @@ template const Visitor &getExampleVisitor() { b.add([](raw_ostream &out, ReturnInst &ret) { out << "visiting ReturnInst: " << ret << '\n'; }); + b.add([](raw_ostream &out, CallInst &ret) { + out << "visiting CallInst: " << ret << '\n'; + }); + b.add([](raw_ostream &out, CallBrInst &ret) { + out << "visiting CallBrInst: " << ret << '\n'; + }); b.addIntrinsic( Intrinsic::umax, [](raw_ostream &out, IntrinsicInst &umax) { out << "visiting umax intrinsic: " << umax << '\n'; From e535e695de0978d8f2ba2c71599c529538a48528 Mon Sep 17 00:00:00 2001 From: tnowicki Date: Wed, 22 Jan 2025 14:51:57 -0500 Subject: [PATCH 6/6] Added CallInst and CallBr to example lit-test --- test/example/visitor-basic.ll | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/example/visitor-basic.ll b/test/example/visitor-basic.ll index 919a429..49a336c 100644 --- a/test/example/visitor-basic.ll +++ b/test/example/visitor-basic.ll @@ -17,6 +17,9 @@ ; DEFAULT-NEXT: %q = ; DEFAULT-NEXT: visiting umin (set): %vm = call i32 @llvm.umin.i32(i32 %v1, i32 %q) ; DEFAULT-NEXT: visiting StringAttrOp: Hello world! +; DEFAULT-NEXT: visiting CallInst: %0 = call i32 @op.func(i32 %v1, i32 %q) +; DEFAULT-NEXT: visiting CallBrInst: callbr void @callee() +; DEFAULT-NEXT: to label %continueBB [label %label1BB, label %label2BB] ; DEFAULT-NEXT: visiting Ret (set): ret void ; DEFAULT-NEXT: visiting ReturnInst: ret void ; DEFAULT-NEXT: inner.counter = 1 @@ -40,9 +43,21 @@ entry: call void (...) @xd.ir.write.vararg(i8 %t, i32 %v2, i32 %q) %vm = call i32 @llvm.umin.i32(i32 %v1, i32 %q) call void @xd.ir.string.attr.op(ptr @0) + call i32 @op.func(i32 %v1, i32 %q) + callbr void @callee() to label %continueBB [label %label1BB, label %label2BB] ret void + +continueBB: + br label %entry + +label1BB: + br label %entry + +label2BB: + br label %entry } +declare void @callee() declare i32 @xd.ir.read__i32() declare i1 @xd.ir.set.read__i1() declare i32 @xd.ir.set.read__i32() @@ -53,3 +68,4 @@ declare i8 @xd.ir.itrunc__i8(...) declare void @xd.ir.string.attr.op(ptr) declare i32 @llvm.umax.i32(i32, i32) declare i32 @llvm.umin.i32(i32, i32) +declare void @op.func(i32, i32)