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

[Yi-Hong Lyu via Phabricator via llvm-commits]

Yi-Hong.Lyu marked 2 inline comments as done.
Yi-Hong.Lyu 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
+
----------------
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
```


================
Comment at: llvm/test/CodeGen/PowerPC/sh-overflow.mir:50
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
----------------
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.


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

https://reviews.llvm.org/D66991