[PATCH] D66991: [PowerPC] Fix SH field overflow issue

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/test/CodeGen/PowerPC/sh-overflow.mir:1
+# RUN: llc -O3 -mtriple=powerpc64le-unknown-linux-gnu -start-after=phi-node-elimination -ppc-asm-full-reg-names -verify-machineinstrs %s -o - | FileCheck %s
+
----------------
Yi-Hong.Lyu wrote:
> jsji wrote:
> > nemanjai wrote:
> > > jsji wrote:
> > > > `-start-after=phi-node-elimination` ? Why not `start-before=ppc-mi-peepholes`? Does this imply that we rely on some other passes to generate the necessary input?
> > > > 
> > > > Can you come up with one that works with either `start-before=ppc-mi-peepholes` or `-start-after ppc-mi-peepholes -ppc-late-peephole `?
> > > Yes, let's change the test case to have 2 RUN lines. One starts before MI Peephole. The other starts after it. The checks should be the same for both as we should do the right thing in both places.
> > > 
> > > Also, please do not talk about the "current state of things" or "what the patch fixes" in the source/tests. These statements make sense only in the context of the patch now, but later down the road, such statements are meaningless. It should suffice to say something like:
> > > "Ensure we do not attempt to transform this into `srwi $r3, $r3, 0` in the form specified by ISA 3.0b (`rlwinm $r3, $r3, 32 - 0, 0, 31`)"
> > Good point.
> Given the MIR:
> ```
> $ cat special_right_shift32_0.mir
> ---
> name:            test
> alignment:       2
> tracksRegLiveness: true
> registers:
>   - { id: 0, class: gprc }
>   - { id: 1, class: gprc }
>   - { id: 2, class: gprc }
> liveins:
>   - { reg: '$r3', virtual-reg: '%0' }
> machineFunctionInfo: {}
> body:             |
>   bb.0.entry:
>     liveins: $r3
> 
>     %1:gprc = LI 0
>     %0:gprc = COPY $r3
>     %2:gprc = SRW %0, %1
>     $r3 = COPY %2
>     BLR implicit $lr, implicit $rm, implicit $r3
> 
> ...
> 
> ```
> ```
> $ llc -O3 -mtriple=powerpc64-unknown-linux-gnu -start-before=ppc-mi-peepholes special_right_shift32_0.mir -o special_right_shift32_0.before.s
> $ llc -O3 -mtriple=powerpc64-unknown-linux-gnu -start-after=ppc-mi-peepholes -ppc-late-peephole special_right_shift32_0.mir -o special_right_shift32_0.after.s
> $ diff special_right_shift32_0.before.s special_right_shift32_0.after.s
> 15a16
> >       slwi 3, 3, 0
> ```
> All the assembly in special_right_shift32_0.before.s is optimized out and it contains only `blr`[1]. In contrast,  special_right_shift32_0.after.s has expected output[2]. That is, we get different results for the 2 RUN lines.
> 
> [1]
> ```
> $ cat special_right_shift32_0.before.s
>         .text
>         .file   "special_right_shift32_0.mir"
>         .globl  test                    # -- Begin function test
>         .p2align        2
>         .type   test, at function
>         .section        .opd,"aw", at progbits
> test:                                   # @test
>         .p2align        3
>         .quad   .Lfunc_begin0
>         .quad   .TOC. at tocbase
>         .quad   0
>         .text
> .Lfunc_begin0:
>         .cfi_startproc
> # %bb.0:                                # %entry
>         blr
>         .long   0
>         .quad   0
> .Lfunc_end0:
>         .size   test, .Lfunc_end0-.Lfunc_begin0
>         .cfi_endproc
>                                         # -- End function
> 
>         .section        ".note.GNU-stack","", at progbits
> 
> ```
> [2]
> ```
> $ cat special_right_shift32_0.after.s
>         .text
>         .file   "special_right_shift32_0.mir"
>         .globl  test                    # -- Begin function test
>         .p2align        2
>         .type   test, at function
>         .section        .opd,"aw", at progbits
> test:                                   # @test
>         .p2align        3
>         .quad   .Lfunc_begin0
>         .quad   .TOC. at tocbase
>         .quad   0
>         .text
> .Lfunc_begin0:
>         .cfi_startproc
> # %bb.0:                                # %entry
>         slwi 3, 3, 0
>         blr
>         .long   0
>         .quad   0
> .Lfunc_end0:
>         .size   test, .Lfunc_end0-.Lfunc_begin0
>         .cfi_endproc
>                                         # -- End function
> 
>         .section        ".note.GNU-stack","", at progbits
> ```
I believe we do difference opts depends on `PostRA`, that maybe why you can not get exact results with above two RUN line.


================
Comment at: llvm/test/CodeGen/PowerPC/sh-overflow.mir:50
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
----------------
Yi-Hong.Lyu wrote:
> jsji wrote:
> > Yi-Hong.Lyu wrote:
> > > jsji wrote:
> > > > nit: `alignment` and `maxAlignment` seems random here, any reason you want to use 1 here?
> > > The special_right_shift32_0 is derived from
> > > ```
> > > unsigned int test(unsigned int a, unsigned int b) {
> > >   return a >> b;
> > > }
> > > ```
> > > generated by `clang --target=powerpc-unknown-unknown`. In contrast, the special_right_shift64_0 is  derived from
> > > ```
> > > unsigned long test(unsigned long a, unsigned long b) {
> > >   return a >> b;
> > > }
> > > ```
> > > generated by `clang --target=powerpc64-unknown-unknown`. I just leave `alignment` and `maxAlignment` as it is. What alignment and maxAlignment  do you think it should be?
> > Thanks for explanation.  I don't have specific number in mind, just wondering why it is 1? since it is smaller than 2 above.
> According to https://llvm.org/docs/MIRLangRef.html#simplifying-mir-files:
> ```
> The whole frameInfo section is often unnecessary if there is no special frame usage in the function. 
> ```
> Would remove the whole frameInfo section and leave `alignment:       2 `there for both  cases.
OK for me. Thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66991/new/

https://reviews.llvm.org/D66991