# [CodeGen] Add support for multiple memory operands in MachineInstr::mayAliasClosedPublicActions

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

# Details

Reviewers
 efriedma hfinkel craig.topper dmgreen
Commits
rG7019cea26dfe: [CodeGen] Add support for multiple memory operands in MachineInstr::mayAlias
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.

# Unit TestsFailedView All

TimeTest
60 msLLVM.CodeGen/AArch64::Unknown Unit Message ("")
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/llc < /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/llvm/test/CodeGen/AArch64/merge-store-dependency.ll -mcpu cortex-a53 -mtriple=aarch64-eabi | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/llvm/test/CodeGen/AArch64/merge-store-dependency.ll --check-prefix=A53
60 msLLVM.CodeGen/AArch64::Unknown Unit Message ("")
Script: -- : 'RUN: at line 2'; c:\ws\prod\llvm-project\build\bin\llc.exe < C:\ws\prod\llvm-project\llvm\test\CodeGen\AArch64\merge-store-dependency.ll -mcpu cortex-a53 -mtriple=aarch64-eabi | c:\ws\prod\llvm-project\build\bin\filecheck.exe C:\ws\prod\llvm-project\llvm\test\CodeGen\AArch64\merge-store-dependency.ll --check-prefix=A53
60 msLLVM.CodeGen/ARM::Unknown Unit Message ("")
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/llc -mtriple armeb-eabi -mattr=armv8.2-a,neon,fullfp16 -target-abi=aapcs-gnu -float-abi hard -o - /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/llvm/test/CodeGen/ARM/big-endian-neon-fp16-bitconv.ll | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/llvm/test/CodeGen/ARM/big-endian-neon-fp16-bitconv.ll
100 msLLVM.CodeGen/ARM::Unknown Unit Message ("")
Script: -- : 'RUN: at line 2'; c:\ws\prod\llvm-project\build\bin\llc.exe -mtriple armeb-eabi -mattr=armv8.2-a,neon,fullfp16 -target-abi=aapcs-gnu -float-abi hard -o - C:\ws\prod\llvm-project\llvm\test\CodeGen\ARM\big-endian-neon-fp16-bitconv.ll | c:\ws\prod\llvm-project\build\bin\filecheck.exe C:\ws\prod\llvm-project\llvm\test\CodeGen\ARM\big-endian-neon-fp16-bitconv.ll
90 msLLVM.CodeGen/Thumb2::Unknown Unit Message ("")
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/llc -mtriple=thumbv8.1m.main-none-none-eabi -mattr=+mve.fp -verify-machineinstrs /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/llvm/test/CodeGen/Thumb2/mve-float32regloops.ll -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/llvm/test/CodeGen/Thumb2/mve-float32regloops.ll
View Full Test Results (12 Failed)

### Event Timeline

Kayjukh created this revision.May 18 2020, 2:07 PM
Herald added a project: Restricted Project. May 18 2020, 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.May 18 2020, 5:08 PM
Kayjukh edited the summary of this revision. (Show Details)

Update instruction ordering in failing test cases.

Herald added a subscriber: dmgreen. May 18 2020, 5:08 PM
llvm/lib/CodeGen/MachineInstr.cpp
1280

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

Kayjukh marked an inline comment as done.May 19 2020, 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
1280

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.

llvm/lib/CodeGen/MachineInstr.cpp
1280

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.May 19 2020, 4:47 AM
llvm/lib/CodeGen/MachineInstr.cpp
1280

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

Kayjukh updated this revision to Diff 264862.May 19 2020, 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.May 21 2020, 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.

Herald added a subscriber: MatzeB. May 21 2020, 11:45 AM

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.May 21 2020, 12:48 PM

LGTM with one small change.

llvm/test/CodeGen/X86/instr-sched-multiple-memops.mir
1 ↗(On Diff #265561)

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

This revision is now accepted and ready to land.May 21 2020, 12:48 PM
Kayjukh updated this revision to Diff 265587.May 21 2020, 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.May 21 2020, 1:55 PM

Yes, looks fine.

This revision was automatically updated to reflect the committed changes.