[AArch64][CostModel] Detects that {Extract,Insert}Element at lane 0 have the same cost as other lanes for real instructions that operates on integer types

[mingmingl]

Summary

Currently, {extract,insert}-element has zero cost at lane 0 [1]. However, there is a cost (by fmov instruction at lane 0 [2], or more generally ext/ins instruction) to move values from SIMD registers to GPR registers, when the element is used explicitly as integers.

See https://godbolt.org/z/faPE1nTn8, when fmov is generated for d* register -> x* register conversion.

Implementation-wise, add a private method AArch64TTIImpl::getVectorInstrCostHelper as a helper function. This way, instruction-based method could share the core logic (e.g., returning zero cost if type is legalized to scalar).

[1] https://github.com/llvm/llvm-project/blob/2cf320d41ed708679e01eeeb93f58d6c5c88ba7a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp#L1853
[2] https://github.com/llvm/llvm-project/blob/2cf320d41ed708679e01eeeb93f58d6c5c88ba7a/llvm/lib/Target/AArch64/AArch64InstrInfo.td#L8150-L8157

[nikic]

Changing the cost model (even if it impacts other places) is the right thing to do here. We cannot accept such special cases in LICM.

[mingmingl]

In D128302#3600211, @nikic wrote:

Changing the cost model (even if it impacts other places) is the right thing to do here. We cannot accept such special cases in LICM.

Acknowledged and it makes sense to me. Is it more idiomatic to create a new revision (to update the cost model) or just modify this existing revision?

[davidxl]

Keeping this revision with discussion history in one place sounds fine.

[mingmingl]

For getVectorInstrCost method, optionally pass a pointer of Instruction, and each target can retrieve the (use) context and customize cost estimation. For AArch64 implementation, fall back to use VectorInsertExtractBaseCost rather than regard extract element at index 0 as a free operation.

[davidxl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1900

missing return instruction ?

[mingmingl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1900

This falls through to use ST->getVectorInsertExtractBaseCost() around line 1903. Basically, this patch reduces the case when extracting element at index 0 is considered free (using truck as a baseline).

[davidxl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1900

ok -- my view of the patch hides the line :)

[nikic]

(Not really familiar with cost model APIs.)

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1868

Yes, all users of an instruction are instructions, it's safe to use cast here.

1879

This should probably be just U.getOperandNo() == 0?

[mingmingl]

In AArch64TargetTransformInfo.cpp, change helper function isExtractedElementUsedAsInteger by

1. assert that all users of an instruction are instructions
2. simplify the store operation usage check, since integer could only be directly used as value as it is, the source of store operation (i.e., not the destination to store the value to).
3. require that uitofpinst/sitofpinst is the only use of extract-element if we think the extract-element operation is free.

[mingmingl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1868

thanks for taking a look! I looked at a few existing cpp and they also assume so.

Changing this to an assert then.

1879

I meant to check that the extract-element is used as the value, the source of store operation.

However, upon this comment, I realize that the extracted element could only be the source, since the destination should be a pointer type. Simplified the store operation check.

[mingmingl]

Change to AArch64TargetTransformInfo.cpp only:

1. Add the missing continue for store operation check.
2. Remove the OneUse restriction added for uitofpinst/sitofpinst.

[fhahn]

Hm, do we have precedence for doing a full scan of users in other cost APIs?

[RKSimon]

llvm/include/llvm/CodeGen/BasicTTIImpl.h

92 ↗

(On Diff #440763)

any chance we can add a default (nullptr) arg value and avoid all the nullptr additions?

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1860

!isa_and_nonnull<ExtractElementInst>(Inst) ?

1868

Scanning the uses like this isn't going to be cheap - and getCost calls can be called a LOT.

Why did you decide this was necessary? Do existing tests change without it? You don't appear to have added any test coverage for these cases.

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

37

Add the other CHECKs for other extraction indices?

43

pre-commit the test with trunk codegen and rebase the patch to show the change in cost reporting

[mingmingl]

llvm/include/llvm/CodeGen/BasicTTIImpl.h

92 ↗

(On Diff #440763)

This is a good question.

If we want to have a default (nullptr) arg, each derived class (AArch64TargetTransformInfo, X86TargetTransformInfo, etc) needs to override the default pointer argument, since derived classes won't inherit the default argument of base class's virtual method [1] and it's suggested not to "change the default parameters of overridden inherited functions" [2].

I don't feel strong about this, but not adding a default following the existing style of Index -> Index has a default value of -1 in class TargetTransformInfo, but not in class BasicTTIImpl or derived classes.

May I know your thoughts about this? (leaving this comment open)

[1] https://stackoverflow.com/questions/3533589/can-virtual-functions-have-default-parameters
[2] http://www.gotw.ca/gotw/005.htm

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1868

Scanning cost is a good point; one mitigation (in the patch) is that, there is a break out of the loop once an integer use case if ound.

May I measure the compile time increase with https://github.com/nikic/llvm-compile-time-tracker and report back? (leaving this open)

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

43

will do in another patch (got to reply first for compile-time question around scanning uses())

[mingmingl]

Changes

1. Update commit message to mirror phabricator summary.
2. Use`!isa_and_null`, and added the other CHECKs for other extraction indices, as suggested.

[mingmingl]

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

43

created https://reviews.llvm.org/D128945, with this test case, and another test case in LICM/AArch64/ subdirectory.

Both test cases are going to have diff, and the diff for LICM/AArch64/extract-element.ll would be like https://www.diffchecker.com/RyiE4Zn5 (code of trunk on the left, code of this patch on the right)

[mingmingl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1868

Compared between HEAD~1 and this patch gives https://llvm-compile-time-tracker.com/compare.php?from=1e556f459b44dd0ca4073e932f66ecb6f40fe31a&to=63035f714c94d31698b5b4ab79b9912814c29345&stat=instructions

Across different configs and benchmarks, the compile-time increases or decreases within +/- 0.1%. I wonder if this means it's fine in terms of the compile-time cost?

[mingmingl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1868

Actually I wasn't sure if AArch64 code path is executed at this point (without reading how tracker is implemented) even if the metrics (e.g., instructions) could be collected without running the code. Do you have other suggestions to measure the cost of iterating uses?

Why did you decide this was necessary?

In order to know if there is a scalar integer use (that results in additional operations to move / dup vector subregisters to general purpose registers), all uses of the extract-element instruction are enumerated here.

Do existing tests change without it?

The reason to handle store is based on this example (https://godbolt.org/z/fn4jWr5eW).

You don't appear to have added any test coverage for these cases.

I could add https://godbolt.org/z/fn4jWr5eW as a test case if it sounds good.

[mingmingl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1868

Actually I wasn't sure if AArch64 code path is executed at this point (without reading how tracker is implemented) even if the metrics (e.g., instructions) could be collected without running the code. Do you have other suggestions to measure the cost of iterating uses?

Regarding compile-time change

1. The llvm-compile-time-tracker measures x86 code paths.
2. I did a similar comparison (by scraping scripts) for -DLLVM_TARGETS_TO_BUILD="AArch64" , and build llvm-test-suite with O3
   1. The diff of instruction counts are within +/- 0.1%, and the diff of cycles changes are within 2% (CTMark results uploaded
      test.csv1 KBDownload
      )

Regarding the pattern of iterating uses() and its cycle change

• I think it's not un-common for a pass to iterate uses() of instructions, and here only a subset of extract-element instructions uses are iterated.
• On the other hand, "used-as-integer" fact might be pre-computed and maintained inside class ExtractElementInst, so AArch64TTI can query / re-use the result, if we do want to avoid calculating it repeatedly.

Would like to hear some thoughts for the topics above, thanks!

[mingmingl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1868

Between iterating all uses and not looking at uses, another option for now is to look at use_empty(), since the instruction cost would ultimately be 0 (regardless of index value) if use_empty() returns true.

Together with this option, a FIXME might be added to point out if all uses are store operations that could be performed bitwise (i.e., without cross-register-bank copies), the extract-element operation is free.

[mingmingl]

Rebase on D128945.

[mingmingl]

Address concerns about scanning uses in cost API.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1868

To summarize the question and solution:

Since no full-scan of use exists due to compile-time concerns, simplify the helper function to look at 'use_empty' only, and added a FIXME for store operations.

[davidxl]

I want to suggest a different way of changing the interfaces to make the intention clearer:

1. add a new interface for vectorcost in TTI that takes an instruction (to replace the opcode parameter)
2. for places where context instruction is not available, no changes are needed;
3. in TTIImpl, by default, the instruction based interface is simply a wrapper to the opcode based interface
4. for aarch64, additional logic is added for checking the lane ...

[mingmingl]

Address the comments by an interface change.

[davidxl]

llvm/include/llvm/Analysis/TargetTransformInfo.h

1155 ↗

(On Diff #444348)

Perhaps document this that this is used when the instruction is not available.

1161 ↗

(On Diff #444348)

Mention that this interface is used when I is available (and the interface implementation asserts it is not nullptr).

llvm/lib/Analysis/TargetTransformInfo.cpp

869 ↗

(On Diff #444348)

probably submit this as a NFC patch seperately?

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h

176

what is the purpose of the using statement here?

[mingmingl]

Address comments.

[mingmingl]

llvm/include/llvm/Analysis/TargetTransformInfo.h

1155 ↗

(On Diff #444348)

Done. Mention the typical use as provision vectorization/scalarization cost in vectorizer passes.

llvm/lib/Analysis/TargetTransformInfo.cpp

869 ↗

(On Diff #444348)

Sure. Will leave the FIXME inside this function, and the assert the newly added 'getVectorInstrCost'.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h

176

Oh good catch. This is a mistake. 'using statement' is not needed for AArch64 since there is an override for new interface.

[davidxl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h

176

Is it needed for other targets? The new method is a public method in BasicTTIImplBase, which is inherited and visible in the derived class.

[mingmingl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h

176

Is it needed for other targets?

Yes, and I learnt it by seeing the compile error without 'using statements'.

Basically, the overloaded method in derived class could hide method in the base class, even if function signature is different. There is one example, and one relevant discussion.

Also, renaming the new interface differently also compiles, since name hiding doesn't exist by renaming.

There are using BaseT::getIntImmCost statements in <Target>TargetTransformInfo.cpp (example) (not sure if 'getIntImmCost' still helps with name hiding now, since overtime many targets overload the method with the same function signature)

[davidxl]

Since the use scanning code is removed (which raised some compile time concern), and latest revisions looks good to me. Please wait a little for other reviewers to comment.

[mingmingl]

ping.

[davidxl]

Consider split this into two patches: the first NFC just introduces the new interface (as a simple wrapper to the existing opcode based one). The second one is the functional change.

[mingmingl]

In D128302#3696604, @davidxl wrote:

Consider split this into two patches: the first NFC just introduces the new interface (as a simple wrapper to the existing opcode based one). The second one is the functional change.

Sent out D131114 as NFC.

[mingmingl]

Rebase after NFC.

[mingmingl]

rebase after D131197

[RKSimon]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1981

do you need the this-> ?

[mingmingl]

resolve comments.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1981

Ah this-> is not needed, fixed it.

(Sorry for the delay; I was out in the past week)

[RKSimon]

LGTM but a aarch64 specialist should sign off as well

[fhahn]

Just a general comment: please try to include the full context when uploading a diff: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1981

nit: LLVM coding style uses UpperCase for variables.

1993

I might be missing something here, but the comment here doesn't seem to match the code, which just checks if the result is used. I'd expect that in all interesting cases the result will be used. And if it is not used the cost is not really that interesting, as it is effectively a no-op that should be removed.

[mingmingl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1993

It makes complete sense that instructions without users are not interesting, and [1] is sufficient for this patch (will hold on updating diff for better context). Let me elaborate why this patch and it'd be great to hear your thoughts! (This is a lot of words despite efforts to simplify them.. hopefully structure / bold fonts makes it easier. Thanks for reviewing this patch and reading the replies!)

About the motivating use case

• It is shown in LICM/AArch64/extract-element.ll -> an extract-element instruction shouldn't be considered free (and thereby sinked to loop exit) by LICM when cost kind is TCK_SizeAndLatency -> extract-element has size and latency cost, unless it could be "combined" into its user instructions (e.g., user instruction can access lane directly).
• Say the loop-exit is frequently executed in a hot path, sink extract-element (along with a series of its def instructions) is inefficient.
• Redundancy elimination might be an alternative option but I haven't got a chance to look into the alternative option yet, with the thought that fixing cost model is overall a good change as well :)*

About the motivation of this patch

• When the instruction exists in IR (indicated by function parameter const Instruction& I) and extract result is integer (a stronger signal of a fmov being emitted), this patch use the same cost all lanes (not considering lane 0 free).

So why this patch wants to tell a real vector instruction from a virtual one (by asking for const Instruction&I, not just Opcode)?

• Vectorizer passes have Opcode but not real instructions in its provisioning stage [2]. Not telling real from virtual may make vectorizer result better or worse in a fragile way.
• Plus, a new interface of getVectorInstCost won't mess up the codebase (w.r.t API/code style) when improvements for vectorizer cost models are added in the future.

But why vectorizer gives better or worse result in a fragile way?

• Take BaseTTIImpl::getScalarizationOverhead as an example, it calls getVectorInstrCost element-by-element for virtual instructions. With various SIMD instructions, element-by-element estimation is not accurate for AArch64.
• AArch64TTI doesn't override this method so BaseTTIImpl is used. Just as a reference, x86 backend overrides this method for some specialized cases.
• If this patch is NOT no-op for method getScalarizationOverhead, it could exacerbates the inaccurate cost model (SLP, etc) and misleads vectorizers.
• In practice, when non-zero cost for lane 0 is applied for "provisioned" vector instructions, some IR from llvm/test/Transforms/<vectorizer-subdir>/AArch64/ gets better generated code and some gets worse generated code. I didn't inspect them all, but presumably there are going to be better or worse cases, since without this change vectorizers sometimes gives worse output (e.g., https://godbolt.org/z/c1bjhGdT5 is for llvm/test/Transforms/SLPVectorizer/AArch64/ext-trunc.ll)

A related question, I wonder if it's with a reason (or just unintended status quo) that getVectorInstrCost interface doesn't ask for a CostKind parameter?

[1]

+InstructionCost AArch64TTIImpl::getVectorInstrCost(const Instruction &I,
+                                                   Type *Val, unsigned Index) {
+  InstructionCost cost = getVectorInstrCost(I.getOpcode(), Val, Index);
+
+  // According to NEON programmer guide, other than multiply instructions,
+  // instructions that access scalars can access any element in the vector register.
+  //
+  // The cost of an extract depends on whether the user instruction.
+  // 1. If users could use scalars in vector registers directly (e.g., store), the
+  // extract-element operation is essentially free.
+  // 2. If the user instruction requires core register as operand (i.e.,
+  // cannot use scalars in vector register), an explicit move operation will
+  // be codegen'd.
+  //
+  // 'cost' might be an optimistic 0 when lane is 0.
+  // Returns the base cost (as the same with other lanes) if the scalar is an integer.
+  //
+  // FIXME:
+  // The cost of extract-element from vector is better estimated with context (e.g. users).
+  if (!isa_and_nonnull<ExtractElementInst>(&I) || !Val->getScalarType()->isIntegerTy()) 
+    return ST->getVectorInsertExtractBaseCost();
+  return cost;
+}
+

[2] Basically, vectorizer pass sees one form (scalar or vector) of "physical" instructions (that do exists in the IR), and tries to "provision" the cost of the other form; this means instructions of the other form doesn't exist yet, and only Opcode is known and used to provision the cost.

[mingmingl]

In D128302#3725318, @fhahn wrote:

Just a general comment: please try to include the full context when uploading a diff: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Yeah I will remember to do this from now on (already did it earlier this week upon suggestions from another reviewer, and a partial diff was purely unintentional.) Also will hold on updating the patch to keep the patch context for discussion (leave all comments open).

[davidxl]

I guess fhahn@ meant that for all useful scenarios, the instruction needs to have some uses (otherwise they are dead). For this case, the check should probably be tightened : instead of checking if the instruction has uses, it needs to examine the uses and differentiate integer uses that requires a fmov and other use contexts (which makes the extract noop).

Checking use emptiness can pessimize cases where the cost is zero, but it returns non-zero cost.

[RKSimon]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1993

A related question, I wonder if it's with a reason (or just unintended status quo) that getVectorInstrCost interface doesn't ask for a CostKind parameter?

This is on my TODO list that was waiting for the getInstructionCost refactor (D79483) - but I want to get some other CostKind changes (notably D132216) out of the way first.

[mingmingl]

Add a private method AArch64TTIImpl::getVectorInstrCostHelper as a helper function. This way, instruction-based method could share the core logic (e.g.,
returning zero cost if type is legalized to scalar).

Uploaded with full context and updated all affected test cases.

[mingmingl]

In D128302#3743563, @davidxl wrote:

I guess fhahn@ meant that for all useful scenarios, the instruction needs to have some uses (otherwise they are dead). For this case, the check should probably be tightened : instead of checking if the instruction has uses, it needs to examine the uses and differentiate integer uses that requires a fmov and other use contexts (which makes the extract noop).

Added a FIXME to estimate extract/insert cost with use-def contexts.

Checking use emptiness can pessimize cases where the cost is zero, but it returns non-zero cost.

In D128302#3731045, @mingmingl wrote:

In D128302#3725318, @fhahn wrote:

Just a general comment: please try to include the full context when uploading a diff: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Yeah I will remember to do this from now on (already did it earlier this week upon suggestions from another reviewer, and a partial diff was purely unintentional.) Also will hold on updating the patch to keep the patch context for discussion (leave all comments open).

Uploaded with full context this time.

I'd like to point out the current patch still lacks test coverage for "virtual" vector instructions in print<cost-model> pass, due to the fact that instructions are physical in an IR. Will upload to get preliminary feedback on the overall direction -> it's feasible to get back test coverage, by calling op-based (not instruction based) getVectorInstrCost in print<cost-model> pass (the callsite of op-based cost API could be gated by a boolean option and turned on where it's needed).

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1981

With the refactoring this variable is gone. Updated patch should use UpperCase variables already.

1993

Thanks for making the cost-model changes! (mark this as done).

1993

Resolved by a refactoring that includes code sharing, comment update and dropping the uninteresting case.

Redundancy elimination might be an alternative option but I haven't got a chance to look into the alternative option yet, with the thought that fixing cost model is overall a good change as well :)*

Just to provide an update on this, looked into how to optimize by redundancy elimination this week -> machine-cse does eliminate the redundant machine instructions (and LICM/AArch64/extract-element.ll is supposed to be a reduced test case).

[davidxl]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h

68

Maybe changing 'VirtualInst' to be name 'hasNoRealUse' or flip it to 'hasRealUse'?

[mingmingl]

address comment.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h

68

Change to 'HasRealUse'. An instruction is a derived class of 'User', but I think it's fine to call it a use without ambiguity here.

[davidxl]

The patch looks reasonable to me. Check if @fhahn @RKSimon have further comments.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

1972

VirtualInst--> HasRealUse.

[mingmingl]

fix comment after renaming variable (VirtualInst->HasRealUse).

[RKSimon]

llvm/test/Analysis/CostModel/AArch64/kryo-inseltpoison.ll

9 ↗

(On Diff #458884)

update the comment

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

9

update the comment

[mingmingl]

updated test comments

[mingmingl]

Thanks for reviews! Going to commit.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h

   InstructionCost getCFInstrCost(unsigned Opcode, TTI::TargetCostKind CostKind,
                                  const Instruction *I = nullptr);
 
-  using BaseT::getVectorInstrCost;
   InstructionCost getVectorInstrCost(unsigned Opcode, Type *Val,
                                      unsigned Index);
+  InstructionCost getVectorInstrCost(const Instruction &I, Type *Val,
+                                     unsigned Index);
 
   InstructionCost getMinMaxReductionCost(VectorType *Ty, VectorType *CondTy,
                                          bool IsUnsigned,

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

   return ST->getVectorInsertExtractBaseCost();
 }
 
+InstructionCost AArch64TTIImpl::getVectorInstrCost(const Instruction &I,
+                                                   Type *Val, unsigned Index) {
+  InstructionCost cost = getVectorInstrCost(I.getOpcode(), Val, Index);
+
+  auto IsExtractedElementUsedAsInteger =
+      [Val](const Instruction *const Inst) -> bool {
+    if (!isa_and_nonnull<ExtractElementInst>(Inst) ||
+        !Val->getScalarType()->isIntegerTy())
+      return false;
+
+    // According to NEON programmer guide, other than multiply instructions,
+    // instructions that access scalars can access any element in the register
+    // file.
+    //
+    // The cost of extracting a scalar element from a vector register depends
+    // on how scalar will be used:
+    // 1. If users could use scalars in vector registers directly, the
+    // extract-element operation is essentially free.
+    // 2. If the user instruction requires core register as operand (i.e.,
+    // cannot use scalars in vector register), an explicit move operation will
+    // be codegen'd.
+
+    // FIXME:
+    // Do more accurate cost estimation by analyzing the uses of instruction.
+    return !Inst->use_empty();
+  };
+
+  // 'cost' might be an optimistic 0 when lane is 0.
+  // Returns the base cost if we know an explicit move is needed.
+  return IsExtractedElementUsedAsInteger(&I)
+             ? ST->getVectorInsertExtractBaseCost()
+             : cost;
+}
+
 InstructionCost AArch64TTIImpl::getArithmeticInstrCost(
     unsigned Opcode, Type *Ty, TTI::TargetCostKind CostKind,
     TTI::OperandValueKind Opd1Info, TTI::OperandValueKind Opd2Info,

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

     ; CHECK: cost of 2 {{.*}} insertelement <2 x i64> undef, i64 undef, i32 1
     %t3 = insertelement <2 x i64> undef, i64 undef, i32 0
     %t4 = insertelement <2 x i64> undef, i64 undef, i32 1
-
     ret void
 }
 
 ; CHECK-LABEL: vectorInstrExtractCost
 define i64 @vectorInstrExtractCost(<4 x i64> %vecreg) {
-    
-    ; Vector extracts - extracting each element at index 0 is considered
-    ; free in the current implementation. When extracting element at index
-    ; 2, 2 is rounded to 0, so extracting element at index 2 has cost 0 as 
-    ; well.
-    ;
     ; CHECK: cost of 2 {{.*}} extractelement <4 x i64> %vecreg, i32 1
-    ; CHECK: cost of 0 {{.*}} extractelement <4 x i64> %vecreg, i32 2
+    ; CHECK: cost of 2 {{.*}} extractelement <4 x i64> %vecreg, i32 2
     %t1 = extractelement <4 x i64> %vecreg, i32 1
     %t2 = extractelement <4 x i64> %vecreg, i32 2
     %ele = add i64 %t2, 1
     %cond = icmp eq i64 %t1, %ele
 
-    ; CHECK: cost of 0 {{.*}} extractelement <4 x i64> %vecreg, i32 0
+    ; Vector extracts - extracting each element should have a cost
+    ; if they are used as integers.
+    ;
+    ; CHECK: cost of 2 {{.*}} extractelement <4 x i64> %vecreg, i32 0
     ; CHECK: cost of 2 {{.*}} extractelement <4 x i64> %vecreg, i32 3
     %t0 = extractelement <4 x i64> %vecreg, i32 0
     %t3 = extractelement <4 x i64> %vecreg, i32 3

llvm/test/Transforms/LICM/AArch64/extract-element.ll

 ; CHECK-NEXT:    [[TMP12]] = add i64 [[TMP4]], 1
 ; CHECK-NEXT:    br label [[TMP3]]
 ; CHECK:       .split.loop.exit:
-; CHECK-NEXT:    [[DOTLCSSA7:%.*]] = phi <1 x i64> [ [[TMP8]], [[TMP6]] ]
+; CHECK-NEXT:    [[DOTLCSSA:%.*]] = phi i64 [ [[TMP9]], [[TMP6]] ]
 ; CHECK-NEXT:    [[DOTLCSSA6:%.*]] = phi i64 [ [[TMP4]], [[TMP6]] ]
 ; CHECK-NEXT:    [[DOTPH:%.*]] = phi i1 [ [[TMP5]], [[TMP6]] ]
-; CHECK-NEXT:    [[TMP13:%.*]] = extractelement <1 x i64> [[DOTLCSSA7]], i64 0
-; CHECK-NEXT:    [[TMP14:%.*]] = xor i64 [[TMP13]], -1
-; CHECK-NEXT:    [[TMP15:%.*]] = add i64 [[TMP14]], [[DOTLCSSA6]]
-; CHECK-NEXT:    [[TMP16:%.*]] = icmp uge i64 [[TMP15]], [[TMP1]]
-; CHECK-NEXT:    br label [[TMP17:%.*]]
+; CHECK-NEXT:    [[TMP13:%.*]] = xor i64 [[DOTLCSSA]], -1
+; CHECK-NEXT:    [[TMP14:%.*]] = add i64 [[TMP13]], [[DOTLCSSA6]]
+; CHECK-NEXT:    [[TMP15:%.*]] = icmp uge i64 [[TMP14]], [[TMP1]]
+; CHECK-NEXT:    br label [[TMP16:%.*]]
 ; CHECK:       .split.loop.exit2:
 ; CHECK-NEXT:    [[DOTPH3:%.*]] = phi i1 [ [[TMP5]], [[TMP3]] ]
 ; CHECK-NEXT:    [[DOTPH4:%.*]] = phi i1 [ undef, [[TMP3]] ]
-; CHECK-NEXT:    br label [[TMP17]]
-; CHECK:       17:
-; CHECK-NEXT:    [[TMP18:%.*]] = phi i1 [ [[DOTPH]], [[DOTSPLIT_LOOP_EXIT]] ], [ [[DOTPH3]], [[DOTSPLIT_LOOP_EXIT2]] ]
-; CHECK-NEXT:    [[TMP19:%.*]] = phi i1 [ [[TMP16]], [[DOTSPLIT_LOOP_EXIT]] ], [ [[DOTPH4]], [[DOTSPLIT_LOOP_EXIT2]] ]
-; CHECK-NEXT:    [[TMP20:%.*]] = xor i1 [[TMP18]], true
-; CHECK-NEXT:    [[TMP21:%.*]] = select i1 [[TMP20]], i1 true, i1 [[TMP19]]
-; CHECK-NEXT:    ret i1 [[TMP21]]
+; CHECK-NEXT:    br label [[TMP16]]
+; CHECK:       16:
+; CHECK-NEXT:    [[TMP17:%.*]] = phi i1 [ [[DOTPH]], [[DOTSPLIT_LOOP_EXIT]] ], [ [[DOTPH3]], [[DOTSPLIT_LOOP_EXIT2]] ]
+; CHECK-NEXT:    [[TMP18:%.*]] = phi i1 [ [[TMP15]], [[DOTSPLIT_LOOP_EXIT]] ], [ [[DOTPH4]], [[DOTSPLIT_LOOP_EXIT2]] ]
+; CHECK-NEXT:    [[TMP19:%.*]] = xor i1 [[TMP17]], true
+; CHECK-NEXT:    [[TMP20:%.*]] = select i1 [[TMP19]], i1 true, i1 [[TMP18]]
+; CHECK-NEXT:    ret i1 [[TMP20]]
 ;
   br label %3