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

Hal Finkel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 29 09:48:52 PDT 2019


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:
> > steven.zhang wrote:
> > > hfinkel wrote:
> > > > 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.
> > > This is a good idea to verify the instruction properties as it is really needed. However, need some effort. I will create a follow up issue for this. Thank you.
> > re jsji: Good point. I will update the comments, rename the function name, and remove the store. For other opcode, I think, we need some follow up as hfinkel said to verify them together.
> > 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.
> 
> 
> Yes, good idea for such utility, we should be able to catch quite some hidden bugs with it.
> Yes, good idea for such utility, we should be able to catch quite some hidden bugs with it.

Exactly; I used to do this every once in a while my manually searching through the TableGen-generated files; I suppose that we might be able to do this by literally running TableGen and matching the output, but a separate utility might be better from a build-process-management standpoint and because it won't require rerunning TableGen (which could be slow in Debug mode).



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

https://reviews.llvm.org/D69232





More information about the llvm-commits mailing list