Skip to content

Conversation

@wangpc-pp
Copy link
Contributor

@wangpc-pp wangpc-pp commented Dec 3, 2024

In #83875, we changed the type of Width to LocationSize. To get
the clsuter bytes, we use LocationSize::getValue() to calculate
the value.

But when Width is an unknown size LocationSize, an assertion
"Getting value from an unknown LocationSize!" will be triggered.

This patch simply skips MemOp with unknown size to fix this issue
and keep the logic the same as before.

This issue was found when implementing software pipeliner for
RISC-V in #117546. The pipeliner may clone some memory operations
with BeforeOrAfterPointer size.

In llvm#83875, we changed the type of `Width` to `LocationSize`. To get
the clsuter bytes, we use `LocationSize::getValue()` to calculate
the value.

But when `Width` is an unknown size `LocationSize`, an assertion
"Getting value from an unknown LocationSize!" will be triggered.

This patch simply skips MemOp with unknown size to fix this issue
and keep the logic the same as before.

This issue was found when implementing software pipieliner for
RISC-V in llvm#117546. The pipeliner may clone some memory operations
with `BeforeOrAfterPointer` size.
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM. I assume that this is going to be tested by the following RISCV PR.

@wangpc-pp
Copy link
Contributor Author

Code that will generate unknown size LocationSize:

/// Update the memory operand with a new offset when the pipeliner
/// generates a new copy of the instruction that refers to a
/// different memory location.
void ModuloScheduleExpander::updateMemOperands(MachineInstr &NewMI,
MachineInstr &OldMI,
unsigned Num) {
if (Num == 0)
return;
// If the instruction has memory operands, then adjust the offset
// when the instruction appears in different stages.
if (NewMI.memoperands_empty())
return;
SmallVector<MachineMemOperand *, 2> NewMMOs;
for (MachineMemOperand *MMO : NewMI.memoperands()) {
// TODO: Figure out whether isAtomic is really necessary (see D57601).
if (MMO->isVolatile() || MMO->isAtomic() ||
(MMO->isInvariant() && MMO->isDereferenceable()) ||
(!MMO->getValue())) {
NewMMOs.push_back(MMO);
continue;
}
unsigned Delta;
if (Num != UINT_MAX && computeDelta(OldMI, Delta)) {
int64_t AdjOffset = Delta * Num;
NewMMOs.push_back(
MF.getMachineMemOperand(MMO, AdjOffset, MMO->getSize()));
} else {
NewMMOs.push_back(MF.getMachineMemOperand(
MMO, 0, LocationSize::beforeOrAfterPointer()));
}
}
NewMI.setMemRefs(MF, NewMMOs);
}

LocationSize Width = 0;
if (TII->getMemOperandsWithOffsetWidth(MI, BaseOps, Offset,
OffsetIsScalable, Width, TRI)) {
if (!Width.hasValue())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd somewhat expect this to just collect the unknown width and let the users figure out what to do with it? Would that require widespread changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think we need to modify clusterNeighboringMemOps and other places as well. That can be a future work if you agree.

@wangpc-pp wangpc-pp merged commit db9057e into llvm:main Dec 5, 2024
9 checks passed
@wangpc-pp wangpc-pp deleted the main-sched-mem-cluster-invalid branch December 5, 2024 12:15
RKSimon added a commit that referenced this pull request Dec 6, 2024
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.

3 participants