[PATCH] D85814: [X86][GlobalISel] Legalize G_ICMP results to s8.

Wed Aug 12 10:13:50 PDT 2020

arsenm added a comment.

In D85814#2213559 <https://reviews.llvm.org/D85814#2213559>, @craig.topper wrote:

> In D85814#2212932 <https://reviews.llvm.org/D85814#2212932>, @arsenm wrote:
>
>> 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)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85814/new/

https://reviews.llvm.org/D85814