Differential D138899
[DAGCombiner] handle more store value forwarding Authored by shchenz on Nov 28 2022, 11:13 PM.
Details
When lowering calls on target like PPC, some stack loads will be generated Suggested by @RolandF, this patch removes the unnecessary loads for the
Diff Detail
Event Timeline
Comment Actions Please add some information about other approaches you considered/attempted to solve this problem. I really don't like the idea of doing store forwarding in the peephole. Are there no existing passes that can perform this type of store forwarding (and most likely more general forms of it)? While I am not all that familiar with MemorySSA, it would seem that this is the exact type of thing it should handle effortlessly. Comment Actions Thanks for taking a look @nemanjai
Hope I explained enough for the reason I didn't do this in DAG-ISEL while lower calls with byval parameter. And PPC is working on switching to Global-ISEL, so that's another reason it is not so good to implement the load elimination for byval parameter in DAG-ISEL. My first thought was implementing this in MachineCSE. But I did't do it there because:
MemorySSA is not an analysis pass that can be used by MIR pass for now. And MemorySSA depends on AAResultsWrapperPass which may leads to compiler time concern. IR level pass mem2reg also does not require MemorySSA to do the transformation. I thought there is no other pass which can implement this opt in PPC pipeline except the above ones, but maybe you can give some insight here. Comment Actions There is an existing place in codegen to handle forwarding a store value to the corresponding load - ForwardStoreValueToDirectLoad in lib/CodeGen/SelectionDAG/DAGCombiner.cpp. Handling this in codegen would not require putting the code in multiple places. The code there is relatively simple - it just looks to see if the thing on the chain immediately before the load is the setting store. For the first case in byval-lhs.ll there is a CALLSEQ_START on the chain in between the load and the store. Maybe it is possible to look past that in the chain to see the store. Since the load is for a call there may be a register copy required to replace the load. Where there is a sequence of multiple stores followed by multiple loads it would require looking back in the chain past loads and past stores to fixed stack locations that do not overlap. I don't know if that is allowed, but in theory it could work. I would suggest to discuss offline with @nemanjai if such an approach is viable and preferred. If not, I think that is a reasonable argument for an MIR level approach. Comment Actions address @RolandF comment, fixing in DAG Combiner and add more reviewers for other targets
Comment Actions @tstellar Hi, could you please help to confirm if the AMDGPU case changes are valid? Thank you very much.
Comment Actions Thanks for review @nemanjai .
Comment Actions LGTM. The minor nits do not require another review. I'm not sure if @tstellar has anything further to add or if he objects to the AMDGPU changes.
Comment Actions @shchenz You neither incorporated nor responded to the comments I left when approving the patch. Could you please respond to the two comments as to why you felt they did not need to be incorporated into the final patch? Comment Actions I am really sorry, Nemanja. @nemanjai I remember I addressed your comments and tested the new update when I committed this patch. I must forget to amend the changes to the final commit. 63854f91d3ee1056796a5ef27753648396cac6ec is committed for the two comments. Comment Actions Heads up that we've bisected a test failure (https://bugs.chromium.org/p/chromium/issues/detail?id=1412740) to this commit. We're still investigating but would be interested to hear if others have found any issues too. Comment Actions I believe this is causing a miscompile on this code. $ cat /tmp/a.ll source_filename = "../../chrome/browser/dom_distiller/dom_distiller_service_factory.cc" target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128" target triple = "i686-unknown-linux-android24" %0 = type { %1 } %1 = type { %2 } %2 = type { ptr } %3 = type { %4 } %4 = type { %5 } %5 = type { ptr } %6 = type { %7 } %7 = type { %8 } %8 = type { ptr } %9 = type { %10 } %10 = type { %11 } %11 = type { ptr } @g = external hidden unnamed_addr constant { [5 x ptr], [9 x ptr] }, align 4 @g2 = external hidden unnamed_addr constant { [5 x ptr] }, align 4 ; Function Attrs: minsize nounwind null_pointer_is_valid optsize uwtable define hidden void @f(ptr noundef align 4 dereferenceable_or_null(48) %0, ptr noundef byval(%0) align 4 %1, ptr noundef byval(%3) align 4 %2, ptr noundef byval(%6) align 4 %3, ptr noundef byval(%9) align 4 %4) unnamed_addr #0 align 2 { %6 = alloca %0, align 4 %7 = alloca %3, align 4 %8 = alloca %6, align 4 %9 = alloca %9, align 4 store ptr getelementptr inbounds ({ [5 x ptr] }, ptr @g2, i32 0, inrange i32 0, i32 2), ptr %0, align 4 %10 = getelementptr inbounds i8, ptr %0, i32 4 %11 = load ptr, ptr %1, align 4 store ptr null, ptr %1, align 4 store ptr %11, ptr %6, align 4 %12 = load ptr, ptr %2, align 4 store ptr null, ptr %2, align 4 store ptr %12, ptr %7, align 4 %13 = load ptr, ptr %3, align 4 store ptr null, ptr %3, align 4 store ptr %13, ptr %8, align 4 %14 = load ptr, ptr %4, align 4 store ptr null, ptr %4, align 4 store ptr %14, ptr %9, align 4 tail call void @h1(ptr noundef align 4 dereferenceable_or_null(44) %10, ptr noundef nonnull byval(%0) align 4 %6, ptr noundef nonnull byval(%3) align 4 %7, ptr noundef nonnull byval(%6) align 4 %8, ptr noundef nonnull byval(%9) align 4 %9) #2 store ptr getelementptr inbounds ({ [5 x ptr], [9 x ptr] }, ptr @g, i32 0, inrange i32 0, i32 2), ptr %0, align 4 store ptr getelementptr inbounds ({ [5 x ptr], [9 x ptr] }, ptr @g, i32 0, inrange i32 1, i32 2), ptr %10, align 4 call void @h2(ptr noundef align 4 dereferenceable_or_null(4) %4) #2 call void @h3(ptr noundef align 4 dereferenceable_or_null(4) %3) #2 call void @h4(ptr noundef align 4 dereferenceable_or_null(4) %2) #2 call void @h5(ptr noundef align 4 dereferenceable_or_null(4) %1) #2 ret void } ; Function Attrs: minsize null_pointer_is_valid optsize declare void @h1(ptr noundef align 4 dereferenceable_or_null(44), ptr noundef byval(%0) align 4, ptr noundef byval(%3) align 4, ptr noundef byval(%6) align 4, ptr noundef byval(%9) align 4) unnamed_addr #1 ; Function Attrs: minsize nounwind null_pointer_is_valid optsize uwtable declare hidden void @h2(ptr noundef align 4 dereferenceable_or_null(4)) unnamed_addr #0 align 2 ; Function Attrs: minsize nounwind null_pointer_is_valid optsize uwtable declare hidden void @h3(ptr noundef align 4 dereferenceable_or_null(4)) unnamed_addr #0 align 2 ; Function Attrs: minsize nounwind null_pointer_is_valid optsize uwtable declare hidden void @h4(ptr noundef align 4 dereferenceable_or_null(4)) unnamed_addr #0 align 2 ; Function Attrs: minsize nounwind null_pointer_is_valid optsize uwtable declare hidden void @h5(ptr noundef align 4 dereferenceable_or_null(4)) unnamed_addr #0 align 2 attributes #0 = { minsize nounwind null_pointer_is_valid optsize uwtable "frame-pointer"="non-leaf" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="i686" "target-features"="+cx8,+mmx,+sse,+sse2,+sse3,+ssse3,+x87" "tune-cpu"="generic" } attributes #1 = { minsize null_pointer_is_valid optsize "frame-pointer"="non-leaf" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="i686" "target-features"="+cx8,+mmx,+sse,+sse2,+sse3,+ssse3,+x87" "tune-cpu"="generic" } attributes #2 = { minsize nounwind optsize } !llvm.linker.options = !{} !llvm.module.flags = !{!0, !1, !2, !3, !4, !5, !6} !0 = !{i32 1, !"NumRegisterParameters", i32 0} !1 = !{i32 7, !"Dwarf Version", i32 4} !2 = !{i32 2, !"Debug Info Version", i32 3} !3 = !{i32 1, !"wchar_size", i32 4} !4 = !{i32 8, !"PIC Level", i32 2} !5 = !{i32 7, !"uwtable", i32 2} !6 = !{i32 7, !"frame-pointer", i32 1} $ llc /tmp/a.ll # one commit before this change ... movl 8(%ebp), %ebx movl $g2+8, (%ebx) leal 12(%ebp), %ecx movl (%ecx), %eax movl %ecx, %edx leal 16(%ebp), %ecx movl (%ecx), %ecx movl %eax, -40(%ebp) leal 20(%ebp), %edi movl %ecx, -32(%ebp) movl (%edi), %eax movl %eax, -24(%ebp) leal 24(%ebp), %esi movl (%esi), %eax movl %eax, -16(%ebp) xorps %xmm0, %xmm0 movups %xmm0, (%edx) ; sets 12(%ebp), 16(%ebp), 20(%ebp), 24(%ebp) to 0 leal 4(%ebx), %eax movl -16(%ebp), %ecx movl %ecx, 16(%esp) ; stores -16(%ebp), which is also 24(%ebp), to 16(%esp) movl -24(%ebp), %ecx movl %ecx, 12(%esp) movl -32(%ebp), %ecx movl %ecx, 8(%esp) movl -40(%ebp), %ecx movl %ecx, 4(%esp) movl %eax, (%esp) calll h1@PLT ... $ llc /tmp/a.ll # at this commit ... movl 8(%ebp), %ebx movl $g2+8, (%ebx) leal 12(%ebp), %ecx movl (%ecx), %eax movl %ecx, %edx xorps %xmm0, %xmm0 leal 16(%ebp), %ecx movl (%ecx), %ecx movl %eax, -32(%ebp) leal 20(%ebp), %edi movl %ecx, -24(%ebp) movl (%edi), %eax movl %eax, -16(%ebp) leal 24(%ebp), %esi movups %xmm0, (%edx) ; sets 12(%ebp), 16(%ebp), 20(%ebp), 24(%ebp) to 0 movl (%esi), %eax ; loads 24(%ebp) to %eax, which is now 0 movl %eax, -40(%ebp) leal 4(%ebx), %ecx movl %eax, 16(%esp) ; stores 0 to 16(%esp) which is wrong!!! movl -16(%ebp), %eax movl %eax, 12(%esp) movl -24(%ebp), %eax movl %eax, 8(%esp) movl -32(%ebp), %eax movl %eax, 4(%esp) movl %ecx, (%esp) calll h1@PLT ... see my added comments in the llc output Going to revert this commit (and the follow up commit) Comment Actions To me, this patch is innocent for the failure https://bugs.chromium.org/p/chromium/issues/detail?id=1412740. The patch exposes an issue in the pre-ra machine scheduler on X86 target. Before the machine scheduler: 176B %12:gr32 = LEA32r %fixed-stack.0, 1, $noreg, 0, $noreg 192B %6:gr32 = MOV32rm %12:gr32, 1, $noreg, 0, $noreg :: (dereferenceable load (s32) from %ir.4, align 16) // the content will be stored to 16($esp) is loaded before the filing 0 MOVUPSmr 208B MOVUPSmr %18:gr32, 1, $noreg, 0, $noreg, %3:vr128 :: (store (s128) into %ir.1, align 4) 320B MOV32mr $esp, 1, $noreg, 16, $noreg, %6:gr32 :: (store (s32)) After the machine scheduler: 264B %12:gr32 = LEA32r %fixed-stack.0, 1, $noreg, 0, $noreg 272B MOVUPSmr %18:gr32, 1, $noreg, 0, $noreg, %3:vr128 :: (store (s128) into %ir.1, align 4) 280B %6:gr32 = MOV32rm %12:gr32, 1, $noreg, 0, $noreg :: (dereferenceable load (s32) from %ir.4, align 16) // now the content is loaded after the filling 0 MOVUPSmr 320B MOV32mr $esp, 1, $noreg, 16, $noreg, %6:gr32 :: (store (s32)) Seems like scheduler does not know %6:gr32 = MOV32rm and MOVUPSmr are aliased. @aeubanks Thanks for the quick action. Could you please help to verify if llc option -enable-misched=false can solve your issue? I am not very familiar with X86 target, do you have any suggestions about how to fix this X86 scheduling issue, create a github issue for X86 target? Thank you! Comment Actions I'm not familiar with scheduler, but it looks to me like a general scheduler problem rather than X86 specific. I have filed 60744. Comment Actions
Thanks for creating the bug. Yes, reproducing the issue needs to apply this patch. Comment Actions that bug has been fixed, now this can reland (hopefully assuming the fix isn't reverted for some reason), sorry for the trouble may want to double check the reproducer before relanding Comment Actions I have been waiting for some days for D144711 for potential issues. Since no issues reported, I will reland this patch. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
I think a comment describing this function would be useful. Something like: