This is an archive of the discontinued LLVM Phabricator instance.

Wrong code generation for VSX floating-point loads with fast-isel
ClosedPublic

Authored by uweigand on Jul 21 2016, 10:02 AM.

Details

Summary

See https://llvm.org/bugs/show_bug.cgi?id=28630 for a description of the problem.

There were two locations where fast-isel would generate a LFD instruction with a target register class VSFRC instead of F8RC when VSX was enabled. The first is PPCFastISel::PPCEmitLoad, which had multiple problems:

1.) Typo in the definition of is64VSXLoad:

bool Is32VSXLoad = IsVSSRC && Opc == PPC::LFS;
bool Is64VSXLoad = IsVSSRC && Opc == PPC::LFD;

The second line needs to use isVSFRC (like PPCEmitStore does)

2.) Move creation of resultReg. Even if the above typo is fixed, IsVSFRC would often be wrong, because it is computed from resultReg -- but in many cases, resultReg was still zero at this point.

3.) Once both the above are fixed, we're now generating a VSX instruction -- but an incorrect one, since generation of an indexed instruction with null index is wrong. Fixed by copying the code handling the same issue in PPCEmitStore.

The second place is PPCFastISel::PPCMaterializeFP, where we would emit an LFD to load a constant from the literal pool, and use the wrong result register class. Fixed by hardcoding a F8RC class even on systems supporting VSX.

Diff Detail

Event Timeline

uweigand updated this revision to Diff 64917.Jul 21 2016, 10:02 AM
uweigand retitled this revision from to Wrong code generation for VSX floating-point loads with fast-isel.
uweigand updated this object.
uweigand added reviewers: hfinkel, kbarton.
uweigand added a subscriber: llvm-commits.

I have confirmed that this patch will also fix PR27718

kbarton added inline comments.Aug 4 2016, 2:41 PM
lib/Target/PowerPC/PPCFastISel.cpp
528

Should these two utility routines now change?
In particular, don't need to check for ResultReg != 0, since that should never happen. They can also get the register class ID from the UseRC variable directly, instead of having to look it up through the MRI (lines 149, 152).

uweigand updated this revision to Diff 66943.Aug 5 2016, 6:54 AM
uweigand marked an inline comment as done.
uweigand added inline comments.
lib/Target/PowerPC/PPCFastISel.cpp
528–529

OK, I've changed the helper routines to take a regclass instead of a register. This means we don't even need to move the initialization of ResultReg, since UseRC is already set up correctly in either case here.

kbarton accepted this revision.Aug 5 2016, 7:47 AM
kbarton edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 5 2016, 7:47 AM
This revision was automatically updated to reflect the committed changes.
uweigand marked an inline comment as done.