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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
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
----------------
jsji wrote:
> 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? 
I don't understand this question. SSA == Static Single Assignment form. Since `r4` is live-in **and** defined in the entry block, it is not SSA.

But this is really arguing semantics. I haven't checked, but it is entirely possible that the MachineRegisterInfo does not compute the defs and uses of physical registers when consuming MIR. So if we are providing an MIR test case that we are passing to pre-RA passes, we should write them without the registers already allocated.


================
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:
> `-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`)"


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

https://reviews.llvm.org/D66991