[PATCH] D69232: [PowerPC] Clear the sideeffect bit for those instructions that didn't have the match pattern

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: llvm/test/CodeGen/PowerPC/mi-scheduling.ll:36
+; CHECK-LABEL: sideeffects:%bb.0 entry 
+; CHECK-NOT:Global memory object and new barrier chain: SU({{[0-9]+}}).
+; CHECK:SU({{[0-9]+}}):   renamable $vf{{[0-9]+}} = LXSD 136
----------------
jsji wrote:
> steven.zhang wrote:
> > jsji wrote:
> > > Global memory objects can be `hasOrderedMemoryRef` as well, so having `Global memory object` might NOT always due to `hasUnmodeledSideEffects`.
> > > And it is *NOT* easy to come up the testcase in scheduling that can test the other affected opcode above.
> > > so mi-scheduling might not be a great place to check `hasUnmodeledSideEffects`.
> > > 
> > > Maybe we can check it in peephole-opt with MIR. eg:
> > > 
> > > ```
> > > $ cat tm.mir
> > > name:            sideeffects
> > > alignment:       16
> > > liveins:
> > >   - { reg: '$x3', virtual-reg: '' }
> > > body:             |
> > >   bb.0.entry:
> > >     liveins: $x3
> > >    renamable $vf1 = LXSD 136, renamable $x3 ::
> > > 
> > > ...
> > > 
> > > $ llc -run-pass peephole-opt tm.mir -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 -debug-only=peephole-opt
> > > ********** PEEPHOLE OPTIMIZER **********
> > > ********** Function: sideeffects
> > > NAPhysCopy: blowing away all info due to renamable $vf1 = LXSD 136, renamable $x3
> > > Encountered load fold barrier on renamable $vf1 = LXSD 136, renamable $x3
> > > ```
> > > 
> > > This way, we can easily add more affected opcodes in the MIR tests. What do you think?
> > Your suggestion will definitely work. However, as I am checking NOT having "Global memory object" and it will cover the hasUnmodeledSideEffects and other conditions you mentioned. Personally, I want to check that, LXSD SU didn't have memory dependency with others, not just the sideeffects flag.
> Fair enough, but in that case, you shouldn't call the function: sideeffects, and the comments should be updated to reflect your test points.
> 
> Also this IR can be further reduced to include ONLY LXSD and fewer fadd, store is unnecessary.
> 
> And what is more, we should add more testing for other affected op? Eg: opcode with match patterns like 'LXSIBZX'? Make sure nothing changed?
> 
It's probably best if we just add a utility that dumps all of the instruction properties, and then we can directly test the output of that utility. I'd certainly welcome some follow-up patch which did that.


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

https://reviews.llvm.org/D69232