Skip to content
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

[SYCL][Matrix] Extend W/A for more corner cases of AccessChain usage #16370

Open
wants to merge 2 commits into
base: sycl
Choose a base branch
from

Conversation

YuriPlyakhin
Copy link
Contributor

@YuriPlyakhin YuriPlyakhin commented Dec 14, 2024

The new corner case is: AccessChain is used on arrays of Joint Matrices
Fix for CMPLRLLVM-64465

The new corner case is:
AccessChain is used on arrays of Joint Matrices
and in loops
@YuriPlyakhin YuriPlyakhin marked this pull request as ready for review December 24, 2024 01:24
@YuriPlyakhin YuriPlyakhin requested a review from a team as a code owner December 24, 2024 01:24
Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

lgtm given this is high priority with the condition someone more familiar with the pass will review it before it gets into the oneAPI release, just some nits

if (StopOnGEP && isa<GetElementPtrInst>(GEP))
return V;
V = GEP->getPointerOperand();
} else if (Operator::getOpcode(V) == Instruction::BitCast) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we not use dyn_cast here?

Comment on lines +65 to +66
if (Value *RV = Call->getReturnedArgOperand()) {
V = RV;
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 for sret right? should we assert it's a pointer?

if (auto *Call = dyn_cast<CallBase>(V)) {
if (Value *RV = Call->getReturnedArgOperand()) {
V = RV;
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment explaining this continue?

if (!WrapperMatrixTy)
return nullptr;
TargetExtType *MatrixTy =
dyn_cast<TargetExtType>(WrapperMatrixTy->getElementType(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we could use dyn_cast_or_null to get rid of the if right above

if (!MatrixTy)
return nullptr;
StringRef Name = MatrixTy->getName();
if (Name != MATRIX_TYPE)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we move the getName() call into the if since we never use the Name variable again and IMO its clear whats going on?

// __spirv_AccessChain
// First we check if the argument came from a GEP instruction
GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(
stripPointerCastsAndOffsets(CI->getArgOperand(0), true));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we add an inline comment explaining what the true is, like /*StopOnGEPs=*/true

}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove newline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants