This is an archive of the discontinued LLVM Phabricator instance.

[FastISel] Don't trivially kill extractvalues (PR49467)
ClosedPublic

Authored by nikic on Mar 7 2021, 8:35 AM.

Details

Summary

All extractvalues of the same value at the same index will map to the same register, so even if one specific extractvalue only has one use, we should not mark it as a trivial kill, as there may be more extractvalues later.

Fixes https://bugs.llvm.org/show_bug.cgi?id=49467.

Diff Detail

Event Timeline

nikic created this revision.Mar 7 2021, 8:35 AM
nikic requested review of this revision.Mar 7 2021, 8:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2021, 8:35 AM
alex added a subscriber: alex.Mar 7 2021, 8:38 AM
nagisa added a subscriber: nagisa.Mar 7 2021, 12:04 PM
nagisa added a comment.Mar 7 2021, 1:39 PM

This seems like a reasonable fix, whatever value my review of fastisel code has.

This issue seems likely to show up for other instructions in the future, as well. Some that come to mind as likely to trigger this same bug are: gep, freeze, phi, select, extractelement (not currently supported by fastisel),

spatel added a comment.Mar 8 2021, 8:24 AM

I don't know this well, so a couple of drive-by comments inline.
@craig.topper or @RKSimon should have a look.

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
241

The header comment for this function only says:

/// Test whether the given value has exactly one use.

That should be updated with a little more detail about the logic and usage.

llvm/test/CodeGen/X86/pr49467.ll
3

Does it make sense to specify the point-of-failure in the RUN line more explicitly with something like:

-fast-isel -optimize-regalloc=0
nikic updated this revision to Diff 329047.Mar 8 2021, 9:23 AM

Expand comment on hasTrivialKill(), explicitly specify -fast-isel in test.

nikic added inline comments.Mar 8 2021, 10:15 AM
llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
241

I expanded the comment a bit -- hopefully it makes sense, as I'm also not familiar with this code.

llvm/test/CodeGen/X86/pr49467.ll
3

I've added the -fast-isel flag, but don't think that -optimize-regalloc=0 is really relevant here, in that the problem manifests before that and is caught by -verify-machineinstrs (although FastRA would also assert).

craig.topper added inline comments.Mar 8 2021, 3:15 PM
llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
273–274

Should this list of exception opcodes just be hoisted up to its own if+"return false" with a more directed comment? It's not important that we check I->hasOneUse() before them is it?

nikic updated this revision to Diff 329336.Mar 9 2021, 7:18 AM

Move instruction type checks into separate condition.

nikic marked an inline comment as done.Mar 9 2021, 7:57 AM

This patch is intended as a minimal fix for LLVM 12. For the main branch, I think we should rip out the whole kill tracking in FastISel entirely. FastRA can determine kills itself, and at least in my test, there is no negative compile-time impact to letting it do so. Maybe this used to be different, but currently the hasTrivialKill functionality seems to be unnecessary complexity.

This revision is now accepted and ready to land.Mar 9 2021, 9:05 AM
This revision was landed with ongoing or failed builds.Mar 9 2021, 9:46 AM
This revision was automatically updated to reflect the committed changes.