This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix assert from machine verify pass that unmatched register class about fcmp selection in fast-isel
ClosedPublic

Authored by wuzish on Dec 13 2018, 6:51 PM.

Details

Summary

The way to reproduce the issue:

llc -mtriple powerpc64le-unknown-linux-gnu -fast-isel -O0 < llvm/test/CodeGen/PowerPC/fast-isel-fcmp-nan.ll -verify-machineinstrs

Bad machine code: Illegal virtual register for instruction

function: TestULE
basic block: %bb.0 entry (0x1000a39b158)
instruction: %2:crrc = FCMPUD %1:vsfrc, %3:f8rc
operand 1: %1:vsfrc

Fix assert about missing match between fcmp instruction and register class. We should use vsx related cmp instruction xvcmpudp instead of fcmpu when vsx is opened.

add -verifymachineinstrs option into related test cases to enable the verify pass.

@wschmidt could you please have a look since the last change is related to your part? Thanks.

Diff Detail

Event Timeline

wuzish created this revision.Dec 13 2018, 6:51 PM

Gentle ping...

Gentle ping again

hfinkel added inline comments.Dec 27 2018, 9:11 AM
llvm/lib/Target/PowerPC/PPCFastISel.cpp
917–918

This would be the only place I see here where we're checking hasVSX(). Should we call getRegForValue earlier and then use isVSFRCRegClass? Do we need to do the same thing about with isVSSRCRegClass and PPC::FCMPUS?

I assume that we are fixing this specific instance because we have a test case that breaks if we turn on verification by default. However, I would assume that we actually have a lot of other failures hiding in here which would be uncovered by doing a bootstrap build with -O0. I am not suggesting that we put all the fixes in a single patch, just that we want to do more thorough testing before we turn on MachineFunction verification by default.

llvm/lib/Target/PowerPC/PPCFastISel.cpp
917–918

I would actually suggest we go one step further and leave the switch the way it is but change the opcode after we create the virtual registers based on whether the Src... VRegs are VSSRC/VSFRC. This would also allow us to add an assert if we ever hit a situation where the register classes don't match (i.e. one is VSFRC and the other F8RC, etc.).

wuzish updated this revision to Diff 179991.Jan 2 2019, 7:41 PM

Address comment

wuzish marked 2 inline comments as done.Jan 2 2019, 7:46 PM
wuzish added inline comments.
llvm/lib/Target/PowerPC/PPCFastISel.cpp
917–918

I find it's relative easy and OK to move the getRegForValue earlier. It's hard to make an assert of "one is VSFRC and the other F8RC" since the src2 is an imm result materialized by a load which is a normal f8rc load instead of vsx load instruction, which I think is a limitation now since we don't handle load selection well.

Gentle ping for new update

hfinkel accepted this revision.Jan 7 2019, 9:42 AM
hfinkel added inline comments.
llvm/lib/Target/PowerPC/PPCFastISel.cpp
895

It's best to write this out : "Unsupposed f32 VSX comparison".

Otherwise, LGTM.

This revision is now accepted and ready to land.Jan 7 2019, 9:42 AM
wuzish marked an inline comment as done.Jan 8 2019, 6:06 PM
This revision was automatically updated to reflect the committed changes.
wuzish reopened this revision.Jan 8 2019, 10:10 PM
wuzish marked an inline comment as done.
wuzish added inline comments.
llvm/trunk/lib/Target/PowerPC/PPCFastISel.cpp
895 ↗(On Diff #180779)

Well, it can not be assert. Just return false to let DAG selection handle it.

This revision is now accepted and ready to land.Jan 8 2019, 10:10 PM
wuzish updated this revision to Diff 180807.Jan 9 2019, 2:17 AM
wuzish marked an inline comment as done.

use copy to resolve the mismatch instead of assert.

This revision was automatically updated to reflect the committed changes.