[GlobalISel] Volatile load duplication - isObviouslySafeToFold is too eager

[llvmbot]

Bugzilla Link	42032
Version	trunk
OS	Windows NT
Reporter	LLVM Bugzilla Contributor
CC	@aemerson,@dsandersllvm,@qcolombet

Extended Description

isObviouslySafeToFold reports that any neighbouring instructions can "obviously" be folded.

However, globalisel folds without ensuring deletion (as their values may still be live), and so volatile operations can be duplicated erroneously.
One possible workaround:

bool InstructionSelector::isObviouslySafeToFold(MachineInstr &MI,
MachineInstr &IntoMI) const {
// Volatile accesses require special handling:
if (MI.mayLoadOrStore()) {
for (auto mem : MI.memoperands()) {
if (mem->isVolatile())
return false;
}
}
// Immediate neighbours are already folded.
if (MI.getParent() == IntoMI.getParent() &&
std::next(MI.getIterator()) == IntoMI.getIterator())
return !MI.mayLoadOrStore() && !MI.hasUnmodeledSideEffects() &&
empty(MI.implicit_operands());
}

But then, maybe it's time to start discussing improving this whole part of the pattern matcher? Given that at the moment only neighbouring ld/st are considered at all (and once fixed, not if volatile)...

[llvmbot]

assigned to @aemerson

[aemerson]

Sorry, I should set up some kind of alert when new bugs are filed to GlobalISel here.

[aemerson]

Do you happen to have a failing test case?

[llvmbot]

Hi Amara, I was afraid you'd ask that - despite checking out a fresh copy to try reproducing on x86, I was unable.

Reason: x86 does not have any generated patterns for G_LOAD or G_STORE, let alone folded ones :(.

The problem to me seems to be "is obviously safe to fold IntoMI" is better named as "is obviously safe to move to TargetMI".

This can be seen in the TargetGenGlobalISel.inc files, where GIM_CheckIsSafeToFold is called on each operand - but only the instruction just selected is erased. The other instructions stay around and wait for dead code elimination to be removed, but they won't be if they have side effects.

There is a comment that alludes to this already at line 3000 of GlobalISelEmitter.cpp - "We currently handle this by rejecting all loads." (although globalisel does not reject all loads, rather some targets leave them unpatterned).

For the target I'm on, I've modified GIM_CheckIsSafeToFold to exclude the neighbour check entirely (as side effects must never be left in code), and have added a GIM_CheckAndFoldLoad as the very last check before emitting an instruction. This instruction can then erase the volatile load if it has no other uses, and also scans up the block to allow for load folding. I can provide a patch for that if you like, although I understand right now tablegen isn't generally used for much in the way of complex patterns for globalisel, w/ load folding typically coming at a later peephole pass.

Thank you and hope this helps.

[aemerson]

@​Daniel, this was added during the extload support.

I agree that isObviouslySafeToFold() should be renamed into something more accurate, it's just a way to try to bridge the semantic gap between block-local SDAG and function-wide GISel.

I think adding a volatile and hasUnmodeledSideEffects() check should be sufficient here though. In other cases it should be legal to potentially duplicate instructions.