Skip to content

[SandboxIR] Implement LoadInst #99597

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

Jump to bottom
Merged
merged 1 commit into from
Jul 19, 2024
Merged

[SandboxIR] Implement LoadInst #99597

merged 1 commit into from
Jul 19, 2024

Conversation

vporpo
Copy link
Contributor

@vporpo vporpo commented Jul 19, 2024

This patch implements a LoadInst instruction in SandboxIR. It mirrors llvm::LoadInst.

tschuett
tschuett reviewed Jul 19, 2024
View reviewed changes
llvm/include/llvm/SandboxIR/SandboxIR.h
}

public:
unsigned getUseOperandNo(const Use &Use) const final {
Copy link

Choose a reason for hiding this comment

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

The final confused me. It comes from User . For style and safeness, I would mark it as override first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm why would you mark it as override ? LoadInst won't have subclasses so it should be safe to mark it final.

Copy link

Choose a reason for hiding this comment

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

True. But getUseOperandNo overrides a virtual = 0 from User. Once you mark it override the compiler will do checks for you. override final.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think override final is redundant, and I am not sure it's being used in LLVM. If you use final the compiler will check that you are actually overriding and print an error if not, and on top of that it will also check that it is final.

Copy link

@tschuett tschuett Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed. I found one occurrence.

Copy link

Choose a reason for hiding this comment

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

Now that LoadInst is final, the finals are kind of redundant. Feel free to ignore, but override would be 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.

Aren't they still useful though? If you misspell a final function name you will still get an error, but you won't get one if you just rely on the class being marked final.
Well, I think I would prefer to stick with final because it conveys both that it's overriden and that it's final.

Copy link

Choose a reason for hiding this comment

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

No worries. The class is marked final. You cannot inherit from anyway and overwriting the functions. My only confusion is that final is kind of redundant. override is less confusing and gives the same compiler checks.

tschuett
tschuett reviewed Jul 19, 2024
View reviewed changes
llvm/include/llvm/SandboxIR/SandboxIR.h Outdated
@@ -553,6 +557,50 @@ class Instruction : public sandboxir::User {
#endif
};

class LoadInst : public Instruction {
Copy link

Choose a reason for hiding this comment

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

Is LoadInst final?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link

Choose a reason for hiding this comment

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

You can mark LoadInst as final for documentation and optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

dtcxzyw
dtcxzyw reviewed Jul 19, 2024
View reviewed changes
llvm/lib/SandboxIR/SandboxIR.cpp
switch (cast<llvm::Instruction>(LLVMV)->getOpcode()) {
case llvm::Instruction::Load: {
auto *LLVMLd = cast<llvm::LoadInst>(LLVMV);
It->second = std::unique_ptr<LoadInst>(new LoadInst(LLVMLd, *this));
Copy link
Member

Choose a reason for hiding this comment

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

Please use std::make_unique.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just like the lines above we can't use make_unique because the constructors are private.

@vporpo
Copy link
Contributor Author

vporpo commented Jul 19, 2024

I moved getName() outside #ifdef NDEBUG, because that's how it is in LLVM IR.

@vporpo
Copy link
Contributor Author

vporpo commented Jul 19, 2024

Pushed a forward declaration of LoadInst, which should fix the windows build.

@vporpo
Copy link
Contributor Author

vporpo commented Jul 19, 2024

Removed operator<< since it's already implemented in the base class. And also updated the constructor's comment.

alinas
alinas approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@alinas alinas left a comment

Choose a reason for hiding this comment

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

LGTM.

@vporpo
[SandboxIR] Implement LoadInst
093d6ee 
This patch implements a `LoadInst` instruction in SandboxIR.
It mirrors `llvm::LoadInst`.
@vporpo vporpo merged commit 63625f4 into llvm:main Jul 19, 2024
5 of 6 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jul 19, 2024

LLVM Buildbot has detected a new failure on builder clang-debian-cpp20 running on clang-debian-cpp20 while building llvm at step 2 "Checkout the source code".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/108/builds/1476

Here is the relevant piece of the build log for the reference:

Step 2 (Checkout the source code) failure: update (failure)
git version 2.43.0
fatal: unable to access 'https://github.com/llvm/llvm-project.git/': Could not resolve host: github.com
fatal: unable to access 'https://github.com/llvm/llvm-project.git/': Could not resolve host: github.com

yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
@vporpo @yuxuanchen1997
[SandboxIR] Implement LoadInst (#99597)
64e5b37 
Summary:
This patch implements a `LoadInst` instruction in SandboxIR. It mirrors
`llvm::LoadInst`.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251256
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tschuett tschuett tschuett left review comments

@dtcxzyw dtcxzyw dtcxzyw left review comments

@alinas alinas alinas approved these changes

@slackito slackito Awaiting requested review from slackito

@echristo echristo Awaiting requested review from echristo

@aeubanks aeubanks Awaiting requested review from aeubanks

@tmsri tmsri Awaiting requested review from tmsri

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vporpo @llvm-ci @alinas @tschuett @dtcxzyw