Page MenuHomePhabricator

[PowerPC][Altivec] Fix offsets for vec_xl and vec_xst
AcceptedPublic

Authored by nemanjai on Jun 20 2019, 6:20 PM.

Details

Summary

As we currently have it implemented in altivec.h, the offsets for these two intrinsics are element offsets. The documentation in the ABI (as well as the implementation in both XL and GCC) states that these should be byte offsets.

Diff Detail

Repository
rC Clang

Event Timeline

nemanjai created this revision.Jun 20 2019, 6:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2019, 6:20 PM
Herald added a subscriber: kbarton. · View Herald Transcript
jsji added a comment.Jun 21 2019, 3:04 PM

A few questions.

lib/Headers/altivec.h
16364–16365

Why we name it Adjusted? Why not just __addr?

16365

Why we want to cast it to (signed short *) again? Looks like unnecessary casting to me?

test/CodeGen/builtins-ppc-xl-xst.c
5

Any difference for results without power8-vector , except for test9 and test10?

Why not split test9 and test10 to another file for simplicity?

nemanjai marked 3 inline comments as done.Jun 22 2019, 10:08 AM
nemanjai added inline comments.
lib/Headers/altivec.h
16364–16365

Sure. I don't really have any preference with respect to the name at all.

16365

Argh, yup the double cast is silly. I initially did something different for this and just missed cleaning up these. I'll update.

test/CodeGen/builtins-ppc-xl-xst.c
5

I like running all of them both with and without power8-vector. I can simplify this by using check-prefixes=CHECK,CHECK-P8 so that we only have one sequence of checks for each function.

nemanjai updated this revision to Diff 206123.Jun 22 2019, 10:28 AM

Remove the double cast. Simplify the test case. Rename the temp.

jsji accepted this revision.Jun 22 2019, 1:52 PM

LGTM. Thanks for fixing.

This revision is now accepted and ready to land.Jun 22 2019, 1:52 PM