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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

I am OK with you using virtual regs, and the reduced case looks mostly good to me, but I think we should try to avoid rely on other opts before peephole.



================
Comment at: llvm/test/CodeGen/PowerPC/SH-field-overflow.mir:10
+  ; PowerPC Pre-Emit Peephole converts
+  ;   renamable $r4 = LI 0
+  ;   renamable $r5 = SRW renamable $r3, renamable $r4
----------------
Yi-Hong.Lyu wrote:
> jsji wrote:
> > Yi-Hong.Lyu wrote:
> > > jsji wrote:
> > > > We know what exactly we want to test in MIR, so why don't we reduce this MIR test further ?
> > > > eg:  a MIR with following lines should be sufficient for 32 bit, you can add another module for 64 bits, and that should be all.
> > > > 
> > > > ```
> > > > $ cat sh-overflow.mir
> > > > ---
> > > > name:            special_right_shift32_0
> > > > liveins:
> > > >   - { reg: '$x3'}
> > > >   - { reg: '$x4'}
> > > > tracksRegLiveness: true
> > > > body:             |
> > > >   bb.0.entry:
> > > >     liveins: $r3, $r4
> > > >   
> > > >     renamable $r4 = LI 0
> > > >     renamable $r3 = SRW renamable $r3, renamable $r4
> > > >     BLR8 implicit $lr8, implicit $rm, implicit $x3
> > > > ...
> > > > 
> > > > ```
> > > > 
> > > Multiple passes after ppc-mi-peepholes relies on SSA form. Apparently, the test case is not in SSA form so we can't just use it.
> > Good point, sorry, I did not check SSA constraint for MIR carefully. 
> > My example above is mostly just to demo the idea of how to reduce the case
> > 
> > How about something like these?
> > 
> > ```
> > $ cat t2.mir
> > ---
> > name:            special_right_shift32_0
> > liveins:
> >   - { reg: '$r4'}
> >   - { reg: '$r5'}
> > tracksRegLiveness: true
> > body:             |
> >   bb.0.entry:
> >     liveins: $r5, $r4
> >   
> >     renamable $r4 = LI 0
> >     renamable $r3 = SRW killed renamable $r5, killed renamable $r4, implicit-def $x3
> >     BLR8 implicit $lr8, implicit $rm, implicit killed $x3
> > ...
> > $ cat t3.mir
> > ---
> > name:            special_right_shift64_0
> > liveins:
> >   - { reg: '$r4'}
> >   - { reg: '$x5'}
> > tracksRegLiveness: true
> > body:             |
> >   bb.0.entry:
> >     liveins: $r4, $x5
> >   
> >     renamable $r4 = LI 0
> >     renamable $x3 = SRD killed renamable $x5, killed renamable $r4
> >     BLR8 implicit $lr8, implicit $rm, implicit killed $x3
> > ...
> > ```
> Thanks for your test case. Both of your test cases works. One concern is that they are non-SSA MIR tests but we apply some SSA MIR passes (e.g., Live Variable Analysis) on them. I come up with reduced SSA MIR tests, how do you think?
Why they are non-SSA? 


================
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
+
----------------
`-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 `?


================
Comment at: llvm/test/CodeGen/PowerPC/sh-overflow.mir:50
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
----------------
nit: `alignment` and `maxAlignment` seems random here, any reason you want to use 1 here?


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

https://reviews.llvm.org/D66991