[PATCH] D32706: [AArch64] Consider lengthening instructions in cast cost calculation

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added a comment.

Thanks Kristof! Responses inline.



================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:195-196
+  //
+  // TODO: Add additional lengthening operations (e.g., mul, shl, etc.) once we
+  //       verify that the extends are eliminated during code generation.
+  auto *SingleUser = cast<Instruction>(*I->user_begin());
----------------
kristof.beyls wrote:
> If the idea is that this function only return true if the lengthening/widening is zero-cost; maybe the function name should be isLengtheningFree instead of isLengthening?
> Or something similar indicating the expected cost of widening is zero?
Sounds good! I like isLengtheningFree.


================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.h:46-47
 
+  bool isLengthening(Type *Dst, Type *Src, const Instruction *I);
+
 public:
----------------
kristof.beyls wrote:
> I think the term "widening" is used for this operation, both in the LLVM code base, as well as in the ARMARM.
> So, probably best to use isWidening here too.
>From what I've read, ARM uses "lengthening" to refer to the case where both operands are doublewords (e.g., uaddl) and "widening" to refer to the case when only one operand is a doubleword (e.g., uaddw). This patch only deals with the lengthening cases. We might handle the widening cases in the future, but they may not be as straightforward because the operand order matters. I can add a comment here explaining all of this.


================
Comment at: test/Analysis/CostModel/AArch64/lengthening-casts.ll:1-14
+; RUN: opt < %s -cost-model -analyze | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64--linux-gnu"
+
+; CHECK-LABEL: uaddl_8h
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: %tmp0 = zext <8 x i8> %a to <8 x i16>
----------------
kristof.beyls wrote:
> Next to tests that verify that a 0 cost is returned, maybe it'd also be useful to explicitly test that the widening instruction variant actually gets generated?
We actually already have code generation tests for all of these cases over in test/CodeGen/AArch64 (see arm64-vadd.ll for the uaddl cases, for example). I just copied them over here to test the cost model. I'm fine adding an additional "llc" run-line to this test, though, that verifies the code generation property in this context. I checked and there are other cost model tests that do this (e.g., X86/scalarize.ll), so this wouldn't be that unusual.


https://reviews.llvm.org/D32706