This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Initial VSX intrinsic support, with min/max for vector double
ClosedPublic

Authored by wschmidt on Oct 23 2014, 4:33 PM.

Details

Summary

Now that we have initial support for VSX, we can begin adding intrinsics for programmer access to VSX instructions. This patch performs the necessary enablement in the front end, and tests it by implementing intrinsics for minimum and maximum using the vector double data type.

The main change in the front end is to no longer disallow "vector" and "double" in the same declaration (lib/Sema/DeclSpec.cpp), but "vector" and "long double" must still be disallowed. The new intrinsics are accessed via vec_max and vec_min with changes in lib/Headers/altivec.h. Note that for v4f32, we already access corresponding VMX builtins, but with VSX enabled we should use the forms that allow all 64 vector registers.

The new builtins are defined in include/clang/Basic/BuiltinsPPC.def.

I've added a new test as test/CodeGen/builtins-ppc-vsx.c that is similar to, but much smaller than, builtins-ppc-altivec.c. This is intended to grow substantially over time, and allows us to test VSX IR generation without duplicating CHECK lines for the existing bazillion Altivec tests.

Since vector double is not legal, I've removed the expected error messages saying that it isn't from the existing tests test/Parser/altivec.c and test/Parser/cxx-altivec.cpp; and I've changed the expected error messages for 'vector long double'.

There is a companion patch for LLVM that will be reviewed separately; see http://reviews.llvm.org/D5948.

echristo: I added you as a reviewer, but please feel free to nominate a front-end personage to take your place. I'm not sure who should review the decl-spec parsing changes.

Diff Detail

Event Timeline

wschmidt updated this revision to Diff 15362.Oct 23 2014, 4:33 PM
wschmidt retitled this revision from to [PowerPC] Initial VSX intrinsic support, with min/max for vector double.
wschmidt updated this object.
wschmidt edited the test plan for this revision. (Show Details)
wschmidt added reviewers: hfinkel, seurer, echristo.
wschmidt added subscribers: Unknown Object (MLST), wschmidt, hfinkel and 2 others.
hfinkel added inline comments.Oct 23 2014, 11:21 PM
lib/Sema/DeclSpec.cpp
693

XL's docs say:

Note: The vector unsigned long long, vector bool long long, vector signed long long, and vector double types require architectures that support the VSX instruction set extensions, such as POWER7. You must specify the -qarch=pwr7 compiler option when you use these types.

I think it makes sense to do the corresponding thing in Clang. We should only make the VSX types available when PPCTargetInfo::getTargetDefines defines VSX, otherwise we should restrict to the Altivec types.

My motivation for saying this is that getting front-end syntax errors with nice error messages is preferable to getting 'cannot select' errors from the backend.

Hal, totally agree -- bad miss on my part. I'll fix that up.

wschmidt updated this revision to Diff 15434.Oct 24 2014, 1:12 PM

This version now produces different error messages depending on whether or not VSX is enabled. If so, vector double is fine; if not, it's an error. I didn't have access to the TargetInfo information inside DeclSpec::SetTypeSpecType(), so I ended up having to pass that in from the caller. To minimize the amount of churn, I created a new overload of that function and only used the new version for the handling of double. I am certainly open to ideas of better ways to do this; this was the least invasive method I could think of.

hfinkel edited edge metadata.Oct 26 2014, 1:06 AM

I'd like to change the existing error message for use of double with __vector to also say something like, "; did you intend compile for the POWER7?"

Instead of adding a new overload, I think it would be better to just move the necessary type-checking logic into DeclSpec::Finish. DeclSpec::Finish already gets a Preprocessor &PP argument, and Preprocessor has a getTargetInfo function that you can use. The current scheme which depends on knowing to call the correct overload when adding DeclSpec::TST_double seems very fragile.

Just recording a discussion point from LLVMdev: Doing the check in DeclSpec::Finish() will work fine, but we need to be careful to get the caret for the diagnostic in the right place. This will require adding a ninth location that will track where the "double" showed up in the decl, and using that loc appropriately in the diagnostic calls.

wschmidt updated this revision to Diff 15573.Oct 30 2014, 10:23 AM
wschmidt edited edge metadata.

Thanks for the suggestion, Hal -- this looks much cleaner to me. As a side benefit, I was able to clear up the weird behavior of giving both an error and a not-really-pertinent warning for "vector long double." There also wasn't an issue with the caret position, as I was able to just use the TST location in DeclSpec::Finish.

Let me know whether the wording of the new error message for vector double is ok.

hfinkel accepted this revision.Oct 30 2014, 1:55 PM
hfinkel edited edge metadata.

LGTM, thanks!

include/clang/Basic/DiagnosticParseKinds.td
345

We're using the capitalization convention, right?

requires a target CPU of POWER7 or later

(might as well not skimp on the articles)

On the other hand, this could get a little misleading if compiling with -mcpu=power7 -mno-vsx, so maybe we should say:

requires VSX support to be enabled (available on the POWER7 or later)
This revision is now accepted and ready to land.Oct 30 2014, 1:55 PM

We can go with POWER7. I came up with power7 because that is what we accept from -mcpu= on the command line. But I like your last suggestion, so let's go with that.

wschmidt closed this revision.Oct 31 2014, 12:30 PM

Thanks for the reviews and discussion! r220989.