Page MenuHomePhabricator

[CodeGen] Add support for multiple memory operands in MachineInstr::mayAlias
ClosedPublic

Authored by Kayjukh on Mon, May 18, 2:07 PM.

Details

Summary

To support all targets, the mayAlias member function needs to support instructions with multiple operands.

This revision also changes the order of the emitted instructions in some test cases.

Diff Detail

Event Timeline

Kayjukh created this revision.Mon, May 18, 2:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 18, 2:07 PM

I'd like to see an MIR test that shows the aliasing check actually working correctly.

Kayjukh updated this revision to Diff 264758.Mon, May 18, 5:08 PM
Kayjukh edited the summary of this revision. (Show Details)

Update instruction ordering in failing test cases.

dmgreen added inline comments.Tue, May 19, 12:39 AM
llvm/lib/CodeGen/MachineInstr.cpp
1289

It would seem incorrect to return false here now? Same for the pseudo values above.

Kayjukh marked an inline comment as done.Tue, May 19, 3:05 AM

I'd like to see an MIR test that shows the aliasing check actually working correctly.

I will have to find something that fits in a test case and covers this case. Working on it!

llvm/lib/CodeGen/MachineInstr.cpp
1289

Yes, we should only return if the result is true. For the pseudo-values I guess we can just skip the checks entirely and only update the value of SameVal.
It leads to more test cases failing because of instruction ordering. I will update the diff as soon as I fixed all of these.

dmgreen added inline comments.Tue, May 19, 3:26 AM
llvm/lib/CodeGen/MachineInstr.cpp
1289

If the code before worked with two mmo's, would it be possible to do something like:

auto HasAlias = [](MMOa, MMOb) {
  old code as it was before return true/false;
};

for (auto &MMOa : memoperands()) {
  for (auto &MMOb : Other.memoperands()) {
    if (HasAlias(MMOa, MMOb))
      return true;
return false;
Kayjukh marked an inline comment as done.Tue, May 19, 4:47 AM
Kayjukh added inline comments.
llvm/lib/CodeGen/MachineInstr.cpp
1289

It would definitely make things clearer. Thanks for your suggestion!

Kayjukh updated this revision to Diff 264862.Tue, May 19, 5:03 AM

Simplify the method's control flow by wrapping the old dependency checking code in a helper lambda and calling it for each pair of memory operands.

I'd like to see an MIR test that shows the aliasing check actually working correctly.

I will have to find something that fits in a test case and covers this case. Working on it!

@efriedma Except for some variations in instruction scheduling caused by the calls to mayAlias when adding chain dependencies, I cannot seem to find anything that could make for a good test case. I tried looking into the AArch64LoadStoreOptimizer as well as the ARMLoadStoreOptimizer but none of my attempts triggered the

// FIXME: Need to handle multiple memory operands to support all targets.
if (!hasOneMemOperand() || !Other.hasOneMemOperand())
  return true;

code path, even when compiling fairly complex codes for those targets.

Handcrafting a MIR test file that would trigger this code path proves to be quite challenging since we have no direct control over the propagation of memory operands. Did you have a specific test scenario in mind?

On ARM specifically, operations don't usually more than one memoperand, with the exception of load store paired/multiple. So yes, I can see it would be hard to trigger outside of scheduling.

Maybe we could add some debug output to the scheduler showing when it does/does not add a dependency, and check that. So it would be checking scheduling, but not the final schedule.

Kayjukh updated this revision to Diff 265561.Thu, May 21, 11:45 AM

Add some debug output to the instruction sheduler to signal wether or not a chain dependency has been added between two given instructions. This change allows us to properly test the effect of handling multiple memory operands in alias queries on instruction scheduling.

This update also adds a test case that makes use of the newly added scheduler debug output.

On ARM specifically, operations don't usually more than one memoperand, with the exception of load store paired/multiple. So yes, I can see it would be hard to trigger outside of scheduling.

Maybe we could add some debug output to the scheduler showing when it does/does not add a dependency, and check that. So it would be checking scheduling, but not the final schedule.

Following your suggestion, I added some debug output to the instruction scheduler. It makes it way easier to test the changes as we don't have to go through a rarely executed code path to see a change in the output.

efriedma accepted this revision.Thu, May 21, 12:48 PM

LGTM with one small change.

llvm/test/CodeGen/X86/instr-sched-multiple-memops.mir
2

"# REQUIRES: asserts"; release builds don't support the "-debug" flag.

This revision is now accepted and ready to land.Thu, May 21, 12:48 PM
Kayjukh updated this revision to Diff 265587.Thu, May 21, 1:30 PM
Kayjukh marked an inline comment as done.

Update FileCheck directives in

llvm/test/CodeGen/ARM/cortex-a57-misched-vldm-wrback.ll
llvm/test/CodeGen/ARM/cortex-a57-misched-vstm-wrback.ll
llvm/test/CodeGen/ARM/cortex-a57-misched-vstm.ll

to avoid matching the new instruction scheduler debug output.

@efriedma Thanks for your feedback! I had to make some minor adjustments to ARM test cases. If it's still good for you, I will land the patch.

efriedma accepted this revision.Thu, May 21, 1:55 PM

Yes, looks fine.

This revision was automatically updated to reflect the committed changes.