Page MenuHomePhabricator

PowerPC: Fix SPE f64 VAARG handling.
Needs RevisionPublic

Authored by jhibbits on Oct 27 2019, 12:50 PM.

Details

Reviewers
nemanjai
hfinkel
joerg
stefanp
Group Reviewers
Restricted Project
Summary

SPE follows soft-float ABI for doubles, including VAARG passing. For
soft-float, doubles are bitcast to i64, but for SPE they are not, so we
need to perform GPR alignment explicitly for SPE f64.

Event Timeline

jhibbits created this revision.Oct 27 2019, 12:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2019, 12:50 PM
jhibbits added a reviewer: Restricted Project.Dec 5 2019, 9:55 AM
emaste added a subscriber: emaste.Dec 5 2019, 9:55 AM

I think that for f64 on SPE the GprIndex you are computing it going to be ignored. (See my comment...)

Also, would it be possible to add a test case to go with it?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3099

You are computing a new GprIndex above for MVT::f64 on SPE.
However, down here you don't use the newly computed GprIndex because VT.isInteger() is going to return false so you are going to get FprIndex instead.
The same thing happens later on in this function where we use VT.isInteger() ? GprIndex : FprIndex again.
It looks like the newly computed GprIndex is not going to be used since you are always going to be getting FprIndex when you have MVT::f64.

stefanp requested changes to this revision.Dec 5 2019, 11:25 AM
This revision now requires changes to proceed.Dec 5 2019, 11:25 AM
jhibbits marked an inline comment as done.Dec 5 2019, 12:59 PM

Hi Stefan, thanks for the review. You're right this and the other two reviews (D69483, D69484) need tests. @kthomsen created these, I'm trying to marshall them in. He has some tests, but not in a state that's committable yet, and is working on that/looking for help to create reduced cases.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3099

Oops, you're right, I need to update RegConstant as well.