This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix altivec feature on pwr cpus pre pwr6.
Needs ReviewPublic

Authored by sfertile on Nov 30 2020, 7:20 AM.

Details

Reviewers
nemanjai
ZarkoCA
Summary

The altivec feature was an optional part of the 2.03 (pwr4) and 2.04 (pwr5) ISAs, and it was not implemented by any IBM CPUs until Power6.

Diff Detail

Event Timeline

sfertile created this revision.Nov 30 2020, 7:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2020, 7:20 AM
sfertile requested review of this revision.Nov 30 2020, 7:20 AM

The altivec feature was an optional part of the 2.03 (pwr4) and 2.04 (pwr5) ISAs, and it was not implemented by any IBM CPUs until Power6.

The Power Processing Element in various IBM CPUs (including the Cell Broadband Engine) incorporate VMX. With this change, which of the CPU options should a user targeting a PPE use?

The altivec feature was an optional part of the 2.03 (pwr4) and 2.04 (pwr5) ISAs, and it was not implemented by any IBM CPUs until Power6.

The Power Processing Element in various IBM CPUs (including the Cell Broadband Engine) incorporate VMX. With this change, which of the CPU options should a user targeting a PPE use?

I'm not really familiar with PPEs or CELL, but my understanding was that the generic ppc64 was what they targeted. From your question I'm guessing that understanding is wrong. If the best CPU to target for a PPE is one of the ones modified in this patch, they can still do so, but will have to explicitly add -mattr=+altivec when invoking llc. IR with '-mattr=+altivec,... will continue to enable altivec as well. If there are no objections I'll add -mattr=+altivec' to the vector-popcnt-128-ult-ugt.ll test, and add a test showing that the altivec attribute continues to work on pwr4/5.

If the best CPU to target for a PPE is one of the ones modified in this patch, they can still do so, but will have to explicitly add -mattr=+altivec when invoking llc.

That sounds okay.

If there are no objections I'll add `-mattr=+altivec' to the vector-popcnt-128-ult-ugt.ll test, and add a test showing that the altivec attribute continues to work on pwr4/5.

No objections here.

llvm/lib/Target/PowerPC/PPC.td
532–533

Remove the last sentence here.

sfertile updated this revision to Diff 308702.Dec 1 2020, 10:26 AM
  • Added -mattr=+altivec to vector pop count test to keep pwr5 testing
  • Added a test to show enabling altivec through function attribute.
sfertile added inline comments.Dec 2 2020, 6:46 AM
llvm/lib/Target/PowerPC/PPC.td
532–533

I think we need to keep a comment indicating the Power Systems didn't implement the optional extension to avoid having them enabled again in the future.

llvm/lib/Target/PowerPC/PPC.td
532–533

Yup, that's fine. I saw that you changed it to say "IBM Power system", which I guess is correct (the QS blade servers are "Cell BE" as opposed to "Power" systems).