Page MenuHomePhabricator

[PowerPC] [Clang] Enable float128 feature on VSX targets
Needs ReviewPublic

Authored by qiucf on Dec 7 2020, 9:54 PM.

Details

Reviewers
stefanp
nemanjai
steven.zhang
jsji
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

qiucf created this revision.Dec 7 2020, 9:54 PM
steven.zhang added inline comments.Dec 8 2020, 7:40 PM
clang/lib/Basic/Targets/PPC.cpp
350

I think, we can support the float128 as far as the altivec is enabled. Do you have any specific reason on why limit the cpu ?

qiucf added inline comments.Dec 8 2020, 7:43 PM
clang/lib/Basic/Targets/PPC.cpp
350

Do you have any specific reason on why limit the cpu?

Actually no. I also think we can enable for earlier targets. (GCC documented it's enabled for all targets with VSX) Previously, I thought we did not tested targets <P8 well.

steven.zhang added inline comments.Dec 8 2020, 7:52 PM
clang/lib/Basic/Targets/PPC.cpp
350

The backend implementation only depends on altivec. I think it is better to align with backend implementation. And check the cpu will also have problem if we have something like: -mcpu=pwr8 -mattr=-altivec

qiucf updated this revision to Diff 313239.Dec 21 2020, 9:15 PM
qiucf retitled this revision from [PowerPC] [Clang] Enable float128 feature on POWER 8 to [PowerPC] [Clang] Enable float128 feature on Altivec targets.
qiucf edited the summary of this revision. (Show Details)

Align with Altivec targets.

nemanjai requested changes to this revision.Dec 28 2020, 7:56 AM

I understand that the back end only needs Altivec to support the operations, but enabling this on non-vsx subtargets is different from what GCC does. I don't see a good reason for us to depart from that.

This revision now requires changes to proceed.Dec 28 2020, 7:56 AM

I understand that the back end only needs Altivec to support the operations, but enabling this on non-vsx subtargets is different from what GCC does. I don't see a good reason for us to depart from that.

Do we know why GCC guard it under VSX instead of Altivec ? Maybe, it will help us get more clear on what we should align with (GCC's implementation or what it should be).

qiucf updated this revision to Diff 327378.Tue, Mar 2, 1:04 AM
qiucf retitled this revision from [PowerPC] [Clang] Enable float128 feature on Altivec targets to [PowerPC] [Clang] Enable float128 feature on VSX targets.
qiucf edited the summary of this revision. (Show Details)
qiucf set the repository for this revision to rG LLVM Github Monorepo.

Align with GCC, enable float128 for VSX and later subtargets.

But besides, GCC can enable it by -mcpu=power6 -mvsx, this revision did not do it.

Herald added a project: Restricted Project. · View Herald TranscriptTue, Mar 2, 1:04 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
qiucf updated this revision to Diff 327388.Tue, Mar 2, 1:45 AM

Fix redundant code

steven.zhang accepted this revision.Wed, Mar 3, 11:32 PM

LGTM now. But please hold on for at least one week to see if @nemanjai has concern.