[AArch64] Add cost tests for bitreverse

[Rin]

Summary

This patch includes cost tests for bit reverse as well as some adjustments to the cost model.

[dmgreen]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

259

I think that these types should have a slightly higher cost than just 1 (or 1*LT.first, the legalization cost).

It's common in these cost functions to have a table of operand and type, giving a cost to each. That way they can be controlled a little better. Like the getShuffleCost at the end of this file. Would it be worth adding something similar here?

(We could also maybe optimize the v4i32 case better using a vrev32. But it's probably best to use the current cost over what we expect it could be later).

[Rin]

Add adequate costs for bitreverse operations

[dmgreen]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

302

Can we change it to only call getTypeLegalizationCost once.

llvm/test/Analysis/CostModel/AArch64/bitreverse-cost-model.ll

9

I see this doesn't give a cost of 2. I think it is trying to use the legal type that i8 will be converted to (i32) for the costs.

Maybe.. can we add something that says "if the original element type is not the same as the legal element type, add 1"?

llvm/test/Analysis/CostModel/AArch64/bitreverse.ll

2 ↗

(On Diff #350250)

I'm not sure why this testcase was added when there was already one in review adding the same thing. It seems strange. Especially as it missed half of the important cases.

Can you combine the two tests into one? Making sure we have the tests from both. Less boilerplate in them would be good too.

[CarolineConcatto]

Hey @Rin,
I don't think you need bitreverse-cost-model.ll, you can use only bitreverse.ll.
Why some costs are not being changed in bitreverse.ll?

llvm/test/Analysis/CostModel/AArch64/bitreverse-cost-model.ll

3

Do you need to use

-cost-kind=code-size -mtriple=aarch64-none-eabi

?

I believe you can have the same command as bitreverse.ll, no?

nit:
s/%s --check-prefix=SIZE/ %s /

[Rin]

In D102755#2807464, @CarolineConcatto wrote:

Hey @Rin,
I don't think you need bitreverse-cost-model.ll, you can use only bitreverse.ll.
Why some costs are not being changed in bitreverse.ll?

Hi Carol, yeah, you're right, I'll remove it. And I'm working on making the costs adequate as well, which will require adjustments in bitreverse.ll

[Rin]

Address review comments

[CarolineConcatto]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

302

nit:
s/(LegalisationCost.second)/LegalisationCost.second/

305

Is the '+1' added because the ICA.getID() is informing the type is MVT::i32?
If so, then should we remove these types from the table

{Intrinsic::bitreverse, MVT::i8, 2},
{Intrinsic::bitreverse, MVT::i16, 2}

?
If not(the problem is not with the wrong type being selected on the table) can you add an explanation why the table does not solve the problem?

[Rin]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

305

Is the '+1' added because the ICA.getID() is informing the type is MVT::i32?

If so, then should we remove these types from the table

Yes, that's a good point. I'll remove them.

I think David mentioned the reason why the table does not sole the problem. Because it's trying to use the legal type that i8 will be converted to (i32) for the costs. Which is why I added that if. I'll add a comment in the code to highlight that.

[Rin]

Remove unused typed from cost model table

[Rin]

Address nit comment

[dmgreen]

Costs look good, as far as I can tell.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

306

Should this be if (Entry) return LegalisationCost.first * Entry->Cost; now?

[Rin]

Address review comment

[Rin]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

306

Yeah, you're right, I have changed it.

[dmgreen]

Thanks. LGTM

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

case Intrinsic::usub_sat: {
     // This is a base cost of 1 for the vadd, plus 3 extract shifts if we
     // need to extend the type, as it uses shr(qadd(shl, shl)).
     unsigned Instrs =
         LT.second.getScalarSizeInBits() == RetTy->getScalarSizeInBits() ? 1 : 4;
     if (any_of(ValidSatTys, [&LT](MVT M) { return M == LT.second; }))
       return LT.first * Instrs;
     break;
   }
+  case Intrinsic::bitreverse: {
+    static const auto ValidBitTys = {MVT::v8i16, MVT::v2i32, MVT::v4i32};
+    auto LT = TLI->getTypeLegalizationCost(DL, RetTy);
+    if (any_of(ValidBitTys, [&LT](MVT M) { return M == LT.second; }))
+      return LT.first;
+    break;
+  }
   case Intrinsic::abs: {
     static const auto ValidAbsTys = {MVT::v8i8,  MVT::v16i8, MVT::v4i16,
                                      MVT::v8i16, MVT::v2i32, MVT::v4i32,
                                      MVT::v2i64};
     auto LT = TLI->getTypeLegalizationCost(DL, RetTy);
     if (any_of(ValidAbsTys, [&LT](MVT M) { return M == LT.second; }))
       return LT.first;
     break;
static Optional<Instruction *> processPhiNode(InstCombiner &IC,
                                               IntrinsicInst &II) {
   SmallVector<Instruction *, 32> Worklist;
   auto RequiredType = II.getType();
 
   auto *PN = dyn_cast<PHINode>(II.getArgOperand(0));
   assert(PN && "Expected Phi Node!");
 
   // Don't create a new Phi unless we can remove the old one.
   if (!PN->hasOneUse())
     return None;
 
   for (Value *IncValPhi : PN->incoming_values()) {
     auto *Reinterpret = dyn_cast<IntrinsicInst>(IncValPhi);
     if (!Reinterpret ||
         Reinterpret->getIntrinsicID() !=
             Intrinsic::aarch64_sve_convert_to_svbool ||
         RequiredType != Reinterpret->getArgOperand(0)->getType())
       return None;
   }
 
   // Create the new Phi

llvm/test/Analysis/CostModel/AArch64/bitreverse-cost-model.ll

+; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py
+; RUN: opt -cost-model -analyze -cost-kind=code-size -mtriple=aarch64-none-eabi < %s | FileCheck %s --check-prefix=SIZE
+
+declare i8 @llvm.bitreverse.i8(i8) readnone
+
+define i8 @cost8(i8 %a) {
+; SIZE-LABEL: 'cost8'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: %b = call i8 @llvm.bitreverse.i8(i8 %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret i8 %b
+;
+  %b = call i8 @llvm.bitreverse.i8(i8 %a)
+  ret i8 %b
+}
+
+declare i16 @llvm.bitreverse.i16(i16) readnone
+
+define i16 @cost16(i16 %a) {
+; SIZE-LABEL: 'cost16'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: %b = call i16 @llvm.bitreverse.i16(i16 %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret i16 %b
+;
+  %b = call i16 @llvm.bitreverse.i16(i16 %a)
+  ret i16 %b
+}
+
+declare i32 @llvm.bitreverse.i32(i32) readnone
+
+define i32 @cost32(i32 %a) {
+; SIZE-LABEL: 'cost32'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: %b = call i32 @llvm.bitreverse.i32(i32 %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret i32 %b
+;
+  %b = call i32 @llvm.bitreverse.i32(i32 %a)
+  ret i32 %b
+}
+
+declare i64 @llvm.bitreverse.i64(i64) readnone
+
+define i64 @cost64(i64 %a) {
+; SIZE-LABEL: 'cost64'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: %b = call i64 @llvm.bitreverse.i64(i64 %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret i64 %b
+;
+  %b = call i64 @llvm.bitreverse.i64(i64 %a)
+  ret i64 %b
+}
+
+declare <8 x i8> @llvm.bitreverse.v8i8(<8 x i8>) readnone
+
+define <8 x i8> @cost8x8(<8 x i8> %a) {
+; SIZE-LABEL: 'cost8x8'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: %b = call <8 x i8> @llvm.bitreverse.v8i8(<8 x i8> %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret <8 x i8> %b
+;
+  %b = call <8 x i8> @llvm.bitreverse.v8i8(<8 x i8> %a)
+  ret <8 x i8> %b
+}
+
+declare <16 x i8> @llvm.bitreverse.v16i8(<16 x i8>) readnone
+
+define <16 x i8> @cost16x8(<16 x i8> %a) {
+; SIZE-LABEL: 'cost16x8'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: %b = call <16 x i8> @llvm.bitreverse.v16i8(<16 x i8> %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret <16 x i8> %b
+;
+  %b = call <16 x i8> @llvm.bitreverse.v16i8(<16 x i8> %a)
+  ret <16 x i8> %b
+}
+
+declare <4 x i16> @llvm.bitreverse.v4i16(<4 x i16>) readnone
+
+define <4 x i16> @cost4x16(<4 x i16> %a) {
+; SIZE-LABEL: 'cost4x16'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 22 for instruction: %b = call <4 x i16> @llvm.bitreverse.v4i16(<4 x i16> %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret <4 x i16> %b
+;
+  %b = call <4 x i16> @llvm.bitreverse.v4i16(<4 x i16> %a)
+  ret <4 x i16> %b
+}
+
+declare <8 x i16> @llvm.bitreverse.v8i16(<8 x i16>) readnone
+
+define <8 x i16> @cost8x16(<8 x i16> %a) {
+; SIZE-LABEL: 'cost8x16'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: %b = call <8 x i16> @llvm.bitreverse.v8i16(<8 x i16> %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret <8 x i16> %b
+;
+  %b = call <8 x i16> @llvm.bitreverse.v8i16(<8 x i16> %a)
+  ret <8 x i16> %b
+}
+
+declare <2 x i32> @llvm.bitreverse.v2i32(<2 x i32>) readnone
+
+define <2 x i32> @cost2x32(<2 x i32> %a) {
+; SIZE-LABEL: 'cost2x32'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: %b = call <2 x i32> @llvm.bitreverse.v2i32(<2 x i32> %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret <2 x i32> %b
+;
+  %b = call <2 x i32> @llvm.bitreverse.v2i32(<2 x i32> %a)
+  ret <2 x i32> %b
+}
+
+declare <4 x i32> @llvm.bitreverse.v4i32(<4 x i32>) readnone
+
+define <4 x i32> @cost4x32(<4 x i32> %a) {
+; SIZE-LABEL: 'cost4x32'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: %b = call <4 x i32> @llvm.bitreverse.v4i32(<4 x i32> %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret <4 x i32> %b
+;
+  %b = call <4 x i32> @llvm.bitreverse.v4i32(<4 x i32> %a)
+  ret <4 x i32> %b
+}
+
+declare <1 x i64> @llvm.bitreverse.v1i64(<1 x i64>) readnone
+
+define <1 x i64> @cost1x64(<1 x i64> %a) {
+; SIZE-LABEL: 'cost1x64'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: %b = call <1 x i64> @llvm.bitreverse.v1i64(<1 x i64> %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret <1 x i64> %b
+;
+  %b = call <1 x i64> @llvm.bitreverse.v1i64(<1 x i64> %a)
+  ret <1 x i64> %b
+}
+
+declare <2 x i64> @llvm.bitreverse.v2i64(<2 x i64>) readnone
+
+define <2 x i64> @cost2x64(<2 x i64> %a) {
+; SIZE-LABEL: 'cost2x64'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 8 for instruction: %b = call <2 x i64> @llvm.bitreverse.v2i64(<2 x i64> %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret <2 x i64> %b
+;
+  %b = call <2 x i64> @llvm.bitreverse.v2i64(<2 x i64> %a)
+  ret <2 x i64> %b
+}