This is an archive of the discontinued LLVM Phabricator instance.

[PPC] some bugs mainly about sign problem fixed in altivec.h
AbandonedPublic

Authored by Zeson on Nov 30 2016, 3:17 AM.

Details

Summary

It's mainly about signed and unsigned llvm builtins confusion.

  1. When function args are bool vector and signed vector, it should be mapped to signed version instruction. Reference: ALTIVECPIM.pdf
  2. Some logical right shift vector functions should use unsigned >> operator, which would be translated to lshr IR instruction.
  3. Move some test functions needed VSX option support from builtins-ppc-altivec.c to builtins-ppc-vsx.c

Diff Detail

Event Timeline

Zeson updated this revision to Diff 79715.Nov 30 2016, 3:17 AM
Zeson retitled this revision from to [PPC] some bugs mainly about sign problem fixed in altivec.h.
Zeson updated this object.
Zeson added a subscriber: cfe-commits.
nemanjai edited edge metadata.Nov 30 2016, 5:43 AM

Thank you for fixing these issues. I certainly see how the shifts really need to get the signedness right because the right shifts need to fill with the sign bit (so that vector bool will still have all 0 or all 1 bits). However, I don't really follow why the comparisons need to be signed. Could you just elaborate a bit on that?
Other than satisfying my curiosity on that, this LGTM.

Zeson updated this object.Nov 30 2016, 5:44 AM
Zeson edited edge metadata.
Zeson added a comment.Nov 30 2016, 6:02 AM

Thank you for fixing these issues. I certainly see how the shifts really need to get the signedness right because the right shifts need to fill with the sign bit (so that vector bool will still have all 0 or all 1 bits). However, I don't really follow why the comparisons need to be signed. Could you just elaborate a bit on that?
Other than satisfying my curiosity on that, this LGTM.

In my opinion, from the table in the PDF above, it seems that vector bool would be treated as unsigned or signed depending on the other argument type and it would be mapped to related machine instruction. As machine instruction, it is just treated as signed or unsigned operand.

jtony added inline comments.Nov 30 2016, 9:04 AM
lib/Headers/altivec.h
16420–16421

Thanks a lot for your good catch for the macro issue in vec_xst_be, that's a good catch. BTW, Can you move this up also like vec_xst_be?

test/CodeGen/builtins-ppc-vsx.c
1672

I would prefer these definitions occur at the beginning of the file like before.

1682

These test cases should be grouped together with the test cases from 1663 - 1683. Put the vec_xl_be overloads together, and the vec_xst_be together (maybe after vec_xl_be). I am OK with either put these test2 and test3 into test 1, or make them stand-alone, as long as these overloaded test cases for vec_xst_be and vec_xl_be are put together seperately. Thanks for you good catch, this problem is not found in our previous code review.

Zeson updated this revision to Diff 79856.Nov 30 2016, 7:50 PM

Make vec_xst_be and vec_xl_be test cases put together seperately in builtins-ppc-vsx.c
Move up macro __VSX__ to make all vec_xst_be functions included

kbarton requested changes to this revision.Dec 1 2016, 9:13 AM
kbarton edited edge metadata.

Please make explicit the signed for the parameters to the functions you are changing and remove unnecessary casts. I marked the first few that I found, but stopped marking them after the first several.

lib/Headers/altivec.h
13928

The cast for __b is necessary, since it is already a vector signed char.
I don't know whether this will generate superfluous warnings or not, but it's probably best to remove it.

13964–13965

It's better to make the parameter explicitly vector signed short, and remove the cast on the next line, for consistency with other builtins.

14000–14001

same comment - explicitly vector signed int

14039

No cast needed here

14095

No cast needed here

14131–14132

Make signed explicit here

This revision now requires changes to proceed.Dec 1 2016, 9:13 AM
Zeson updated this revision to Diff 80021.Dec 1 2016, 7:48 PM
Zeson edited edge metadata.
Zeson marked 3 inline comments as done.

Remove some unnecessary cast

Zeson added a comment.Dec 1 2016, 8:08 PM

Please make explicit the signed for the parameters to the functions you are changing and remove unnecessary casts. I marked the first few that I found, but stopped marking them after the first several.

I think it does not need to make explicit signed for the parameters such as making vector int to vector signed int, making vector short to vector signed short.
The whole file containing lots of such issues can be modified or fixed in another single patch to avoid introducing noise in this patch.

Zeson marked 2 inline comments as done.Dec 6 2016, 6:35 PM

Hi, All.

The revision has been updated, please review it again.
Thanks a lot.

Sorry, I don't have time to go through the entire patch in detail right now. But I did notice several places where the lines are too long, which need to get fixed.

lib/Headers/altivec.h
14206

line too long

14381

line too long

14549

line too long

@Zeson are you working on an update to this or is this to be abandoned?

Zeson abandoned this revision.Apr 19 2017, 3:24 AM

I think this revision is out-of-date. I'd like to abandon it.