Paths
| Differential D75555
[GlobalISel][Localizer] Enable intra-block localization of already-local uses. ClosedPublic Authored by aemerson on Mar 3 2020, 11:50 AM.
Details
Summary This changes the localizer to attempt intra-block localizer of instructions that have local uses. This is useful because sometimes the entry block itself has many uses of constant-like instructions, which would benefit from shortening live ranges. Previously if an inst had no non-local uses, we wouldn't add it to the list of instructions to attempt further intra-block localization. This gives a 0.7% geomean code size improvement on CTMark.
Diff Detail
Event TimelineThis revision is now accepted and ready to land.Mar 3 2020, 3:38 PM Closed by commit rGe91e1df6ab74: [GlobalISel][Localizer] Enable intra-block localization of already-local uses. (authored by aemerson). · Explain WhyMar 3 2020, 6:30 PM This revision was automatically updated to reflect the committed changes. Comment Actions Hi @aemerson this change caused a regression in lldb testsuite on AArch64/Linux. Apparently stepping gets affected by your change. Kindly revisit. I have temporarily reverted this change. http://lab.llvm.org:8011/builders/lldb-aarch64-ubuntu/builds/2182 This revision is now accepted and ready to land.Mar 4 2020, 2:14 PM This revision now requires changes to proceed.Mar 4 2020, 2:16 PM Comment Actions
That test looks...wrong. single_step_only_steps_one_instruction() looks to be checking that exactly *one* instruction is needed to step between each line of the test code: static void swap_chars() {
g_c1 = '1';
g_c2 = '0';
g_c1 = '0';
g_c2 = '1';
}Before it was just pure luck that the 4 stores were scheduled consecutively: __ZL10swap_charsv: mov w8, #0x31 adrp x9, 9 ; 0x100010000 add x9, x9, #0x240 mov w10, #0x30 adrp x11, 9 ; 0x100010000 add x11, x11, #0x241 strb w8, [x9] strb w10, [x11] strb w10, [x9] strb w8, [x11] ret with this patch, we emit some address computation after the first store: __ZL10swap_charsv: adrp x8, 9 ; 0x100010000 add x8, x8, #0x240 mov w9, #0x31 strb w9, [x8] adrp x10, 9 ; 0x100010000 add x10, x10, #0x241 mov w11, #0x30 strb w11, [x10] strb w11, [x8] strb w9, [x10] ret which is perfectly valid. I'm not sure how to fix this test because even if we change the assertion to be something like assert(step_count <= max_steps), it's still fragile. Comment Actions @omjavaid can you look over the lldb changes? I don't have the hardware to be able to actually run this test but I've tried to relax the checks. Comment Actions
@aemerson I have moved these test to skip list as they dont seem to be valid at least for ARM/AArch64 as you pointed out above. As there already exist a bug against them I ll look into fixing them later. This revision is now accepted and ready to land.Mar 6 2020, 1:31 AM Comment Actions Yep, that test really shouldn't be doing that. Historically, lldb has been avoiding architecture specific artifacts (like assembly) in its tests, but that didn't really work out here. That test has become a nightmare of architecture-specific assertions. If we're going to be testing assembly-level properties, we really should be using assembly inputs. I have now rewritten it to use inline assembly to guarantee a known sequence of instructions. I think the arm assembly is correct (it compiles), but I don't have the hardware to try it on. @omjavaid, if you're able to test that locally, can you remove the skip decorator and give this a spin? Otherwise, I can just remove the decorator and watch the bot... Closed by commit rGc1a97e992da6: Revert "Revert "[GlobalISel][Localizer] Enable intra-block localization of… (authored by aemerson). · Explain WhyMar 6 2020, 9:59 PM This revision was automatically updated to reflect the committed changes. Comment Actions
Hi @labath thanks for the quick fix moving to inline assembly fixed this on AArch64 linux. I have removed skipIf decorators now.
Revision Contents
Diff 248898 llvm/lib/CodeGen/GlobalISel/Localizer.cpp
llvm/test/CodeGen/AArch64/GlobalISel/localizer-arm64-tti.ll
llvm/test/CodeGen/AArch64/GlobalISel/localizer.mir
llvm/test/CodeGen/AArch64/GlobalISel/swifterror.ll
llvm/test/CodeGen/AArch64/GlobalISel/translate-constant-dag.ll
llvm/test/CodeGen/AArch64/tail-call.ll
llvm/test/CodeGen/AArch64/tiny_model.ll
llvm/test/CodeGen/AMDGPU/GlobalISel/constant-bus-restriction.ll
llvm/test/CodeGen/AMDGPU/GlobalISel/extractelement.ll
llvm/test/CodeGen/AMDGPU/GlobalISel/insertelement.ll
llvm/test/CodeGen/AMDGPU/GlobalISel/mubuf-global.ll
llvm/test/CodeGen/AMDGPU/GlobalISel/regbankselect-amdgcn.s.buffer.load.ll
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||