Skip to content

[SandboxIR] Added isVolatile args to existing LoadInst::create function #100850

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 28, 2024
Merged

[SandboxIR] Added isVolatile args to existing LoadInst::create function #100850

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f66ee5e
Added isVolatile args in LoadInst::create function
wizardengineer Jul 27, 2024
7855441
removed some unneeded code and clang format
wizardengineer Jul 27, 2024
8e8c94f
small changes
wizardengineer Jul 28, 2024
f9266b0
added volatile check for NewLd
wizardengineer Jul 28, 2024
587b557
simple fix
wizardengineer Jul 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions llvm/include/llvm/SandboxIR/SandboxIR.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@
#define LLVM_SANDBOXIR_SANDBOXIR_H

#include "llvm/IR/Function.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/IRBuilder.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/User.h"
#include "llvm/IR/Value.h"
#include "llvm/SandboxIR/Tracker.h"
Expand Down Expand Up @@ -769,10 +769,10 @@ class LoadInst final : public Instruction {
unsigned getNumOfIRInstrs() const final { return 1u; }
static LoadInst *create(Type *Ty, Value *Ptr, MaybeAlign Align,
Instruction *InsertBefore, Context &Ctx,
const Twine &Name = "");
bool IsVolatile = false, const Twine &Name = "");
static LoadInst *create(Type *Ty, Value *Ptr, MaybeAlign Align,
BasicBlock *InsertAtEnd, Context &Ctx,
const Twine &Name = "");
bool IsVolatile = false, const Twine &Name = "");
/// For isa/dyn_cast.
static bool classof(const Value *From);
Value *getPointerOperand() const;
Expand Down
12 changes: 6 additions & 6 deletions llvm/lib/SandboxIR/SandboxIR.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,23 +612,23 @@ void BranchInst::dump() const {

LoadInst *LoadInst::create(Type *Ty, Value *Ptr, MaybeAlign Align,
Instruction *InsertBefore, Context &Ctx,
const Twine &Name) {
bool IsVolatile, const Twine &Name) {
llvm::Instruction *BeforeIR = InsertBefore->getTopmostLLVMInstruction();
auto &Builder = Ctx.getLLVMIRBuilder();
Builder.SetInsertPoint(BeforeIR);
auto *NewLI = Builder.CreateAlignedLoad(Ty, Ptr->Val, Align,
/*isVolatile=*/false, Name);
auto *NewLI =
Builder.CreateAlignedLoad(Ty, Ptr->Val, Align, IsVolatile, Name);
auto *NewSBI = Ctx.createLoadInst(NewLI);
return NewSBI;
}

LoadInst *LoadInst::create(Type *Ty, Value *Ptr, MaybeAlign Align,
BasicBlock *InsertAtEnd, Context &Ctx,
const Twine &Name) {
bool IsVolatile, const Twine &Name) {
auto &Builder = Ctx.getLLVMIRBuilder();
Builder.SetInsertPoint(cast<llvm::BasicBlock>(InsertAtEnd->Val));
auto *NewLI = Builder.CreateAlignedLoad(Ty, Ptr->Val, Align,
/*isVolatile=*/false, Name);
auto *NewLI =
Builder.CreateAlignedLoad(Ty, Ptr->Val, Align, IsVolatile, Name);
auto *NewSBI = Ctx.createLoadInst(NewLI);
return NewSBI;
}
Expand Down
14 changes: 13 additions & 1 deletion llvm/unittests/SandboxIR/SandboxIRTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,11 +764,23 @@ define void @foo(ptr %arg0, ptr %arg1) {
// Check create(InsertBefore)
sandboxir::LoadInst *NewLd =
sandboxir::LoadInst::create(Ld->getType(), Arg1, Align(8),
/*InsertBefore=*/Ret, Ctx, "NewLd");
/*InsertBefore=*/Ret, Ctx,
/*IsVolatile=*/false, "NewLd");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is OK for this PR, but having to specify the IsVolatile argument whenever you create non-volatile LoadInsts looks a bit too verbose. The problem is that most of the times you will be creating an instruction with a name, so you can't take advantage of the default value f the IsVolatile=false argument. And swapping IsVolatile with Name isn't what most users will expect.

If you have time, try creating a follow-up PR with two additional create() functions that don't have the IsVolatile argument and remove the default =false value for these ones. So the functions would be:

1. create(Type *Ty, Value *Ptr, MaybeAlign Align, Instruction *InsertBefore, Context &Ctx, bool IsVolatile, const Twine &Name = "")
2. create(Type *Ty, Value *Ptr, MaybeAlign Align, Instruction *InsertBefore, Context &Ctx, const Twine &Name = "")
3. create(Type *Ty, Value *Ptr, MaybeAlign Align, BasicBlock *InsertAtEnd, Context &Ctx, bool IsVolatile, const Twine &Name = "")
4. create(Type *Ty, Value *Ptr, MaybeAlign Align, BasicBlock *InsertAtEnd, Context &Ctx, const Twine &Name = "")

Then in the body of 2 you would just call 1 with /*IsVolatile=*/false, and similarly in the body of 4 you would call 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely understandable in terms of LoadInst looking verbose. Btw, do you still want the bool IsVolatile to be before the Context &Ctx in the follow-up PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that would be even better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion, I'll start on the follow-up PR.

This is OK for this PR

I was a bit confuse on this. Were you suggesting that this PR will get merge first before creating the follow-up NFC PR? If not, then I'll start on the follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's merge this first and then you can work on top of it.

// Checking if create() was volatile
EXPECT_FALSE(NewLd->isVolatile());
EXPECT_EQ(NewLd->getType(), Ld->getType());
EXPECT_EQ(NewLd->getPointerOperand(), Arg1);
EXPECT_EQ(NewLd->getAlign(), 8);
EXPECT_EQ(NewLd->getName(), "NewLd");

sandboxir::LoadInst *NewVLd =
sandboxir::LoadInst::create(Vld->getType(), Arg1, Align(8),
/*InsertBefore=*/Ret, Ctx,
/*IsVolatile=*/true, "NewVLd");

// Checking if create() was volatile
EXPECT_TRUE(NewVLd->isVolatile());
EXPECT_EQ(NewVLd->getName(), "NewVLd");
}

TEST_F(SandboxIRTest, StoreInst) {
Expand Down
Loading