[InstCombine][X86] Improved folding of calls to Intrinsic::x86_sse4a_insertqi.

[andreadb]

Summary

Hi David, Quentin (and all),

This patch teaches the Instruction Combiner how to fold a call to 'Intrinsic::x86_sse4a_insertqi' if the 'length field' (3rd operand) is set to zero, and if the sum between field 'length' and 'bit index' (4th operand) is bigger than 64.

From the AMD64 Architecture Programmer’s Manual:

1. "If the sum of the bit index + length field is greater than 64, the results are undefined."
2. "A value of zero in the field length is defined as a length of 64."

As a consequence of 1. and 2., "If the length field is 0 and the bit index is 0, bits 63:0 of the source operand are inserted. For any other value of the bit index, the results are undefined."

This patch improves the existing combining logic for Intrinsic::x86_sse4a_insertqi adding extra checks to address both point 1. and point 2.

Added extra test cases to existing test 'vec_demanded_elts.ll'.

Please let me know if ok to submit.
Thanks!
Andrea

[qcolombet]

Hi Andrea,

LGTM.

Thanks,
-Quentin

lib/Transforms/InstCombine/InstCombineCalls.cpp

779 ↗

(On Diff #17085)

Maybe add: per AMD64 manual, or something like that.

[majnemer]

lib/Transforms/InstCombine/InstCombineCalls.cpp

784 ↗

(On Diff #17085)

Is there anything that prevents Index + Length from wrapping around? If not, what happens?

[qcolombet]

lib/Transforms/InstCombine/InstCombineCalls.cpp

784 ↗

(On Diff #17085)

I think there is nothing that would prevent that, but in that case we fall into this condition:
"If the sum of the bit index + length field is greater than 64, the results are undefined.”

So I’d say nothing to be done here but maybe I misunderstood something :).

[andreadb]

Hi David and Quentin,

I uploaded a new version of the patch.

Differences with respect to the previous patch are:

• I added the extra comments requested by Quentin.
• I also added a comment that hopefully should answer the question raised by David about the potential wrap around in the sum Index + Length.

Please let me know if ok to submit.

Thanks!
Andrea.

[qcolombet]

Hi Andrea,

LGTM.

Thanks,
-Quentin

[majnemer]

LGTM

llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp

Instruction *InstCombiner::visitCallInst(CallInst &CI) {
   }
 
   case Intrinsic::x86_sse4a_insertqi: {
     // insertqi x, y, 64, 0 can just copy y's lower bits and leave the top
     // ones undef
     // TODO: eventually we should lower this intrinsic to IR
     if (auto CIWidth = dyn_cast<ConstantInt>(II->getArgOperand(2))) {
       if (auto CIStart = dyn_cast<ConstantInt>(II->getArgOperand(3))) {
-        if (CIWidth->equalsInt(64) && CIStart->isZero()) {
+        unsigned Index = CIStart->getZExtValue();
+        // From AMD documentation: "a value of zero in the field length is
+        // defined as length of 64".
+        unsigned Length = CIWidth->equalsInt(0) ? 64 : CIWidth->getZExtValue();
+
+        // From AMD documentation: "If the sum of the bit index + length field
+        // is greater than 64, the results are undefined".
+
+        // Note that both field index and field length are 8-bit quantities.
+        // Since variables 'Index' and 'Length' are unsigned values
+        // obtained from zero-extending field index and field length
+        // respectively, their sum should never wrap around.
+        if ((Index + Length) > 64)
+          return ReplaceInstUsesWith(CI, UndefValue::get(II->getType()));
+
+        if (Length == 64 && Index == 0) {
           Value *Vec = II->getArgOperand(1);
           Value *Undef = UndefValue::get(Vec->getType());
           const uint32_t Mask[] = { 0, 2 };
           return ReplaceInstUsesWith(
               CI,
               Builder->CreateShuffleVector(
                   Vec, Undef, ConstantDataVector::get(
                                   II->getContext(), makeArrayRef(Mask))));

llvm/trunk/test/Transforms/InstCombine/vec_demanded_elts.ll

 define <2 x i64> @testInsertDisjointRange_2(<2 x i64> %v, <2 x i64> %i) {
 ; CHECK:  tail call <2 x i64> @llvm.x86.sse4a.insertqi(<2 x i64> %v, <2 x i64> %i, i8 16, i8 0)
 ; CHECK:  tail call <2 x i64> @llvm.x86.sse4a.insertqi(<2 x i64> %1, <2 x i64> %i, i8 16, i8 32)
   %1 = tail call <2 x i64> @llvm.x86.sse4a.insertqi(<2 x i64> %v, <2 x i64> %i, i8 16, i8 0)
   %2 = tail call <2 x i64> @llvm.x86.sse4a.insertqi(<2 x i64> %1, <2 x i64> %i, i8 16, i8 32)
   ret <2 x i64> %2
 }
 
+; CHECK: define <2 x i64> @testZeroLength(<2 x i64> %v, <2 x i64> %i)
+define <2 x i64> @testZeroLength(<2 x i64> %v, <2 x i64> %i) {
+; CHECK: ret <2 x i64> %i
+  %1 = tail call <2 x i64> @llvm.x86.sse4a.insertqi(<2 x i64> %v, <2 x i64> %i, i8 0, i8 0)
+  ret <2 x i64> %1
+}
+
+; CHECK: define <2 x i64> @testUndefinedInsertq_1(<2 x i64> %v, <2 x i64> %i)
+define <2 x i64> @testUndefinedInsertq_1(<2 x i64> %v, <2 x i64> %i) {
+; CHECK: ret <2 x i64> undef
+  %1 = tail call <2 x i64> @llvm.x86.sse4a.insertqi(<2 x i64> %v, <2 x i64> %i, i8 0, i8 16)
+  ret <2 x i64> %1
+}
+
+; CHECK: define <2 x i64> @testUndefinedInsertq_2(<2 x i64> %v, <2 x i64> %i)
+define <2 x i64> @testUndefinedInsertq_2(<2 x i64> %v, <2 x i64> %i) {
+; CHECK: ret <2 x i64> undef
+  %1 = tail call <2 x i64> @llvm.x86.sse4a.insertqi(<2 x i64> %v, <2 x i64> %i, i8 48, i8 32)
+  ret <2 x i64> %1
+}
+
+; CHECK: define <2 x i64> @testUndefinedInsertq_3(<2 x i64> %v, <2 x i64> %i)
+define <2 x i64> @testUndefinedInsertq_3(<2 x i64> %v, <2 x i64> %i) {
+; CHECK: ret <2 x i64> undef
+  %1 = tail call <2 x i64> @llvm.x86.sse4a.insertqi(<2 x i64> %v, <2 x i64> %i, i8 64, i8 16)
+  ret <2 x i64> %1
+}
 
 ; CHECK: declare <2 x i64> @llvm.x86.sse4a.insertqi
 declare <2 x i64> @llvm.x86.sse4a.insertqi(<2 x i64>, <2 x i64>, i8, i8) nounwind
 
 declare <4 x float> @llvm.x86.avx.vpermilvar.ps(<4 x float>, <4 x i32>)
 define <4 x float> @test_vpermilvar_ps(<4 x float> %v) {
 ; CHECK-LABEL: @test_vpermilvar_ps(
 ; CHECK: shufflevector <4 x float> %v, <4 x float> undef, <4 x i32> <i32 3, i32 2, i32 1, i32 0>