[GlobalIsel][X86] fix legalization of G_CTLZ and G_CTPOP

[tschuett]

Summary

Note that the builders are protected by is64Bit().

More fine-grained availibility checks.

[RKSimon]

llvm/lib/Target/X86/X86LegalizerInfo.cpp

305

We need to split this - with just s64 legality here and s16/s32 in setLegalizerInfo32bit above

And just widen sub 32 cases to s32

[tschuett]

That will need larger refactorings. I have already run into this issue. You cannot have two getActionDefinitionsBuilder with the same operation. If I add popcount to setLegalizerInfo32bit, then the builder will be called twice with the same operation: once for 32 and once for 64 bit.

[tschuett]

if (Subtarget.hasAVX512()) {
} else if (Subtarget.is64Bit()) {
} else if  (Subtarget.is32Bit()) {
}

[RKSimon]

In D150790#4350163, @tschuett wrote:

That will need larger refactorings. I have already run into this issue. You cannot have two getActionDefinitionsBuilder with the same operation. If I add popcount to setLegalizerInfo32bit, then the builder will be called twice with the same operation: once for 32 and once for 64 bit.

It might be separate setLegalizerInfo32bit / setLegalizerInfo64bit isn't actually useful

[tschuett]

That is not what currently is happening.

setLegalizerInfo32bit();
setLegalizerInfo64bit();
setLegalizerInfoSSE1();
setLegalizerInfoSSE2();
setLegalizerInfoSSE41();
setLegalizerInfoSSE42();
setLegalizerInfoAVX();
setLegalizerInfoAVX2();
setLegalizerInfoAVX512();
setLegalizerInfoAVX512DQ();
setLegalizerInfoAVX512BW();

Therefore, I propose the if statements over the features.

[tschuett]

Btw, I am only interested in correctness and low hanging fruit. Coverage is a different issue.

[tschuett]

if (Subtarget.hasAVX512() && Subtarget.is64Bit()) {
// DQ, BW and VLX
} else if (Subtarget.hasAVX2() && Subtarget.is64Bit()) {
} else if (Subtarget.hasAVX() && Subtarget.is64Bit()) {
} else if (Subtarget.hasSSE41() && Subtarget.is64Bit()) {
} else if (Subtarget.hasSSE2() && Subtarget.is64Bit()) {
} else if (Subtarget.hasSSE1() && Subtarget.is64Bit()) {
} else if (Subtarget.is64Bit()) {
} else if (Subtarget.is32Bit()) {
}

Then you don't run in the existing issues of trying to legalize an operation with different features resp. bit width.

[RKSimon]

llvm/lib/Target/X86/X86LegalizerInfo.cpp

305

You should be able to just use legalIf() to support this properly

[tschuett]

llvm/lib/Target/X86/X86LegalizerInfo.cpp

305

I will try this in a followup diff. This diff is only about correctness.

[tschuett]

I looked at the AArch64 legalizer. The use if-statements when they have different features.

[RKSimon]

llvm/lib/Target/X86/X86LegalizerInfo.cpp

305

OK - in which case, lets just use the same pattern as was done for {G_SHL, G_LSHR, G_ASHR} etc. - setLegalizerInfo32bit adds a second copy behind a !Subtarget.is64Bit() check

[tschuett]

ping.

[RKSimon]

LGTM as the SSE42 -> POPCNT/LZCNT fix - assuming you're going to do the legalIf() followup

llvm/lib/Target/X86/X86LegalizerInfo.cpp

getActionDefinitionsBuilder(
     .clampScalar(1, s8, s8);
 
   // Merge/Unmerge
   LegacyInfo.setAction({G_MERGE_VALUES, s128}, LegacyLegalizeActions::Legal);
   LegacyInfo.setAction({G_UNMERGE_VALUES, 1, s128},
                        LegacyLegalizeActions::Legal);
   LegacyInfo.setAction({G_MERGE_VALUES, 1, s128}, LegacyLegalizeActions::Legal);
   LegacyInfo.setAction({G_UNMERGE_VALUES, s128}, LegacyLegalizeActions::Legal);
+
+  if (Subtarget.hasPOPCNT()) {
+    // popcount
+    getActionDefinitionsBuilder(G_CTPOP)
+      .legalFor({{s16, s16}, {s32, s32}, {s64, s64}})
+      .widenScalarToNextPow2(1, /*Min=*/16)
+      .clampScalar(1, s16, s64);
+  }
+
+  if (Subtarget.hasLZCNT()) {
+    // count leading zeros (LZCNT)
+    getActionDefinitionsBuilder(G_CTLZ)
+      .legalFor({{s16, s16}, {s32, s32}, {s64, s64}})
+      .widenScalarToNextPow2(1, /*Min=*/16)
+      .clampScalar(1, s16, s64);
+  }
+
 }
 
 void X86LegalizerInfo::setLegalizerInfoSSE1() {
   if (!Subtarget.hasSSE1())
     return;
 
   const LLT s32 = LLT::scalar(32);
   const LLT s64 = LLT::scalar(64);
void X86LegalizerInfo::setLegalizerInfoSSE41() {
 
   LegacyInfo.setAction({G_MUL, v4s32}, LegacyLegalizeActions::Legal);
 }
 
 void X86LegalizerInfo::setLegalizerInfoSSE42() {
   if (!Subtarget.hasSSE42())
     return;
 
-  const LLT s16 = LLT::scalar(16);
-  const LLT s32 = LLT::scalar(32);
-  const LLT s64 = LLT::scalar(64);
-
-  // popcount
-  getActionDefinitionsBuilder(G_CTPOP)
-    .legalFor({{s16, s16}, {s32, s32}, {s64, s64}})
-    .widenScalarToNextPow2(1, /*Min=*/16)
-    .clampScalar(1, s16, s64);
-
-  // count leading zeros (LZCNT)
-  getActionDefinitionsBuilder(G_CTLZ)
-    .legalFor({{s16, s16}, {s32, s32}, {s64, s64}})
-    .widenScalarToNextPow2(1, /*Min=*/16)
-    .clampScalar(1, s16, s64);
 }
 
 void X86LegalizerInfo::setLegalizerInfoAVX() {
   if (!Subtarget.hasAVX())
     return;
 
   const LLT v16s8 = LLT::fixed_vector(16, 8);
   const LLT v8s16 = LLT::fixed_vector(8, 16);

llvm/test/CodeGen/X86/GlobalISel/legalize-ctpop.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
-# RUN: llc -mtriple=x86_64-linux-gnu -mattr=+sse4.2 -run-pass=legalizer %s -o - | FileCheck %s
+# RUN: llc -mtriple=x86_64-linux-gnu -mattr=+popcnt -run-pass=legalizer %s -o - | FileCheck %s
 
 # test popcount for s16, s32, and s64
 
 ---
 name:            test_ctpop35
 alignment:       16
 legalized:       false
 regBankSelected: false

llvm/test/CodeGen/X86/GlobalISel/legalize-leading-zeros.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
-# RUN: llc -mtriple=x86_64-linux-gnu -mattr=+sse4.2 -run-pass=legalizer %s -o - | FileCheck %s
+# RUN: llc -mtriple=x86_64-linux-gnu -mattr=+lzcnt -run-pass=legalizer %s -o - | FileCheck %s
 
 # test count leading zeros for s16, s32, and s64
 
 ---
 name:            test_ctlz35
 alignment:       16
 legalized:       false
 regBankSelected: false