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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.
Herald added a subscriber: wuzish.

The fix looks good to me.
However, I feel that the test is over-specified.  What you are looking for is a rotate or a shift with an immediate of zero and to make sure that we can safely print those kinds of instructions to assembly. I feel that using the `update_llc_test_checks.py` script only produces a test that will fail when other changes are made later on and forcing the developer of that changeset to regenerate your test.



================
Comment at: llvm/test/CodeGen/PowerPC/convert-rr-to-ri-instrs.ll:5
+target triple = "powerpc64le-unknown-linux-gnu"
+
+define void @special_right_shift32_0() {
----------------
Should add a comment here to say what you are testing. 


================
Comment at: llvm/test/CodeGen/PowerPC/convert-rr-to-ri-instrs.ll:11
+; CHECK-NEXT:    li r4, 0
+; CHECK-NEXT:    slwi r5, r3, 0
+; CHECK-NEXT:    andi. r5, r5, 1
----------------
So, if I understand correctly, this is the instruction that we had trouble printing. 
```
slwi r5, r3, 0
```
I think the test should just look for this instruction and make sure that the zero is there correctly. The test as it stands is over-specified. The tool to generate tests is nice but it creates overhead for the future when scheduling or register allocation changes slightly then tests like this one would need to be updated. 
I think a set of simpler checks might work better. Look for the label (CHECK-LABEL) as you have done. Then look for the `slwi` with the zero making sure you use some kind of wildcard for the register numbers. I would usually also look for the `blr` but you don't have one for this case so that's fine.


================
Comment at: llvm/test/CodeGen/PowerPC/convert-rr-to-ri-instrs.ll:47
+; CHECK-NEXT:    li r4, 0
+; CHECK-NEXT:    rotldi r5, r3, 0
+; CHECK-NEXT:    andi. r5, r5, 1
----------------
Same goes for this. You can simplify the checks quite a bit here.


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

https://reviews.llvm.org/D66991