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.
Paths
| Differential D80161
[CodeGen] Add support for multiple memory operands in MachineInstr::mayAlias ClosedPublic Authored by Kayjukh on May 18 2020, 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 TimelineComment Actions I'd like to see an MIR test that shows the aliasing check actually working correctly.
Comment Actions
I will have to find something that fits in a test case and covers this case. Working on it!
Comment Actions 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. Comment Actions
@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? Comment Actions 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. Comment Actions 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. Comment Actions
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. Comment Actions LGTM with one small change.
This revision is now accepted and ready to land.May 21 2020, 12:48 PM Kayjukh marked an inline comment as done. Comment ActionsUpdate 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. Comment Actions @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. Closed by commit rG7019cea26dfe: [CodeGen] Add support for multiple memory operands in MachineInstr::mayAlias (authored by Kayjukh). · Explain WhyMay 21 2020, 2:06 PM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 265597 llvm/lib/CodeGen/MachineInstr.cpp
llvm/lib/CodeGen/ScheduleDAGInstrs.cpp
llvm/test/CodeGen/AArch64/merge-store-dependency.ll
llvm/test/CodeGen/ARM/big-endian-neon-fp16-bitconv.ll
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
llvm/test/CodeGen/Thumb2/mve-float32regloops.ll
llvm/test/CodeGen/Thumb2/mve-phireg.ll
llvm/test/CodeGen/Thumb2/mve-vst3.ll
llvm/test/CodeGen/Thumb2/umulo-128-legalisation-lowering.ll
llvm/test/CodeGen/X86/instr-sched-multiple-memops.mir
llvm/test/CodeGen/X86/store_op_load_fold2.ll
|
It would seem incorrect to return false here now? Same for the pseudo values above.