Implemented Builtins for vector right and left shift along with appropriate test cases for ISA3.1. Depends on D83516.
saghir nemanjai hfinkel amyk
- Group Reviewers
- rG3136cbe29e74: [PowerPC] Implement Vector Shift Builtins
This will need to be rebased against your 2608 instruction definitions patch.
But yes, I believe you are missing the clang and llc test case for this patch. Requesting changes due to missing tests.
/* vs[l | r | raq] */ (with a new line after the comment)
I think we can remove /* vector shifts for quadwords */.
vector unsigned you mean? For the return of this test and every other unsigned test case.
nit: remove extra space after <
nit: remove after space before ,
Unrelated space change?
Can probably remove these globals as you are not using them in your test.
Just a few nits for this patch.
Edit: Please wait for Amy to give the approval.
Overall seems fine to me, but of course, please wait to hear from Amy.
Just some nits for the test case.
nit: please add a description for test purpose.
nit: The test name and CHECK-LABEL should use test_vec_vslq
+1, if no specific attributes needed. please remove all the #0 and #1
Please also change the function names.
Actually, sorry, I think this comment should be the following instead:
Actually vec_sl seems to be correct. However, that would mean the other functions need to be renamed as the functions are supposed to be: vec_sl, vec_sr, vec_sra. Albion, could you please rename these functions and also ensure your test uses the correct naming.
Add a space after this comment.
Please remove any commented code.
Could you please fix the indentation of the returns to make them all consistent?
No brackets are needed here.
Also, I think it might make sense to move this block into the previous hasP9Altivec condition since in there it has:
setOperationAction(ISD::SHL, MVT::v1i128, Legal); setOperationAction(ISD::SRL, MVT::v1i128, Legal);
There is no need to make a new predicate block, you can put these anonymous patterns in the block above.
I noticed that we have patterns for the PPCISD nodes, but I think no tests to show these patterns?
Oops, I made that comment above since I thought you were putting the block inside a hasP9Altivec block. Sorry.
Maybe you can move this condition right underneath the hasP9Altivec block that contains the SHL/SRL to be closer to those instructions.
My apologies, upon closer inspection I noticed that your tests are in fact the ones with the PPCISD nodes but you are missing the regular shl/srl/sra. Please add the tests for these, as well.