-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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.
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
[SandboxIR] Add utility function to find the base Value for Mem instructions #112030
[SandboxIR] Add utility function to find the base Value for Mem instructions #112030
Conversation
@@ -49,6 +49,16 @@ class Utils { | |||
return const_cast<Instruction *>(I); | |||
} | |||
|
|||
/// \Returns the base Value for load or store instruction \p LSI. | |||
template <typename LoadOrStoreT> | |||
static Value *getMemInstructionBase(const LoadOrStoreT *LSI) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use LLVM's name getUnderlyingObject()
? getMemInstructionBase() is a better name, but since we are just wrapping LLVM's getUnderlyingObject() we should probably stick to the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this does more than just wrap getUnderlyingObject. It also finds the pointer operand.
static_assert(std::is_same_v<LoadOrStoreT, LoadInst> || | ||
std::is_same_v<LoadOrStoreT, StoreInst>, | ||
"Expected sandboxir::Load or sandboxir::Store!"); | ||
return LSI->Ctx.getOrCreateValue( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need getOrCreateValue()
? I assume that sandbox IR would have been generated for the whole function. That would remove the need for friend class Utils
in Context.
auto *V = LSI->Ctx.getValue( getUnderlyingObject(LSI->getPointerOperand()->Val));
assert(V != nullptr && "Missing Sandbox IR value!");
return V;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getUnderlyingObject returns an llvm::Value, getOrCreateValue converts that to a sandboxir::Value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but there is also a public sandboxir::Context::getValue(llvm::Value *)
that converts LLVM IR to Sandbox IR
EXPECT_EQ(sandboxir::Utils::getMemInstructionBase(St2), | ||
sandboxir::Utils::getMemInstructionBase(St3)); | ||
EXPECT_NE(sandboxir::Utils::getMemInstructionBase(St0), | ||
sandboxir::Utils::getMemInstructionBase(St3)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These checks are OK, but we are not actually checking what the returned value actually is.
Some checks like this would also help:
EXPECT_EQ(sandboxir::Utils::getMemInsructionBase(St0), Ctx.getValue(LLVMSt0));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/8098 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/6050 Here is the relevant piece of the build log for the reference
|
No description provided.