This is an archive of the discontinued LLVM Phabricator instance.

[X86][GlobalISel] Legalize G_ICMP results to s8.
ClosedPublic

Authored by craig.topper on Aug 12 2020, 12:52 AM.

Details

Summary

We need to produce a setcc instruction which has an 8-bit result.
This gets rid of a bunch of cases that were using the s1->s8/s16/s32/s64
handling in selectZExt.

I'm not very familiar with GlobalISel yet so I'm not yet sure
the best way to do things. I'd especially like feedback on the
best way to handle the currently split 32-bit and 64-bit mode
handling.

Diff Detail

Event Timeline

craig.topper created this revision.Aug 12 2020, 12:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2020, 12:52 AM
craig.topper requested review of this revision.Aug 12 2020, 12:52 AM
arsenm accepted this revision.Aug 12 2020, 5:27 AM

LGTM. Not sure I understand the question for 32 vs. 64 mode since it doesn't seem relevant for compares

llvm/lib/Target/X86/X86LegalizerInfo.cpp
172

You'll have to do something explicitly to handle non-0 address space pointers eventually, but this is a good enough first step

This revision is now accepted and ready to land.Aug 12 2020, 5:27 AM

LGTM. Not sure I understand the question for 32 vs. 64 mode since it doesn't seem relevant for compares

I have two different getActionDefinitionsBuilder calls with 2 different type lists for G_ICMP right now. One called when !is64BitMode() and one called when is64BitMode(). I feel like I should move up to the constructor so they are closer together instead of being in separate setLegalizerInfo32bit and setLegalizerInfo64Bit functions. Then I'm wondering if there's a good way to use one getActionDefinitionsBuilder and conditionally control the type lists. And not just for G_ICMP. We're missing clampScalar and or other type legalization controls on a lot of operations right now.

LGTM. Not sure I understand the question for 32 vs. 64 mode since it doesn't seem relevant for compares

I have two different getActionDefinitionsBuilder calls with 2 different type lists for G_ICMP right now. One called when !is64BitMode() and one called when is64BitMode(). I feel like I should move up to the constructor so they are closer together instead of being in separate setLegalizerInfo32bit and setLegalizerInfo64Bit functions. Then I'm wondering if there's a good way to use one getActionDefinitionsBuilder and conditionally control the type lists. And not just for G_ICMP. We're missing clampScalar and or other type legalization controls on a lot of operations right now.

I didn't understand why the x86 legalizer rules are structured the way they are last time I looked at them. I think they're overly split up. I also think these were a bit overly aggressive in setting rules before most of the legalizer actions were implemented, and they're using the legacy rule tables. It might be worth just starting over on them (although I'm not sure what the tradeoffs are between the old table rules, and the new actionbuilder is. I only recently discovered the action builder is not merely syntactic sugar for the setAction style of rules)

This revision was landed with ongoing or failed builds.Aug 12 2020, 10:17 AM
This revision was automatically updated to reflect the committed changes.