[PATCH] D120481: [AArch64] Try to re-use extended operand for SETCC with vector ops.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D120481#3634726 <https://reviews.llvm.org/D120481#3634726>, @dmgreen wrote:

> In D120481#3634566 <https://reviews.llvm.org/D120481#3634566>, @fhahn wrote:
>
>> In D120481#3632283 <https://reviews.llvm.org/D120481#3632283>, @dmgreen wrote:
>>
>>> This sounds good as far as I can tell. If we are dealing with a dag combine, do we need to worry about odd types at all?
>>
>> Do you mean worry in terms of whether it is optimal? There are a few test cases with odd types, and it seems like they are handled OK on that front.
>
> I meant for awkward types like i23 and i128. They don't have to be optimal, so long as they work OK. Just so long as we have a couple of tests, to make sure it doesnt run into problems.

Ah got it! I added a few tests with vectors of i13 and i15. Looks like this handled well, because they are first converted to legal vector types.

> The change sounds good with a small simplification suggestion. LGTM





================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18113
+                                        Op->getOperand(0));
+  if (Op0SExt && isSignedIntSetCC(CC)) {
+    Op0ExtV = SDValue(Op0SExt, 0);
----------------
dmgreen wrote:
> I think this might be able to combine the if blocks together for the ne/eq conditions:
> ```
> if (Op0SExt && (isSignedIntSetCC(CC) || isIntEqualitySetCC(CC))) {
>   Op0ExtV = SDValue(Op0SExt, 0);
>   Op1ExtV = DAG.getNode(ISD::SIGN_EXTEND, DL, UseMVT, Op->getOperand(1));
> } else if (Op0ZExt && (isUnsignedIntSetCC(CC) || isIntEqualitySetCC(CC))) {
>   Op0ExtV = SDValue(Op0ZExt, 0);
>   Op1ExtV = DAG.getNode(ISD::ZERO_EXTEND, DL, UseMVT, Op->getOperand(1));
> } else
>   return SDValue();
> ```
Simplified as suggested, thanks!


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18043
+  EVT UseMVT = FirstUse->getValueType(0);
+  if (UseMVT.getScalarType().getScalarSizeInBits() <=
+      Op0MVT.getScalarType().getScalarSizeInBits())
----------------
dmgreen wrote:
> I think this can skip the getScalarType call: UseMVT.getScalarSizeInBits() <= Op0MVT.getScalarSizeInBits()
Thanks, simplified!


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18065
+                                        Op->getOperand(0));
+  if (Op0SExt && isSignedIntSetCC(CC)) {
+    Op0ExtV = SDValue(Op0SExt, 0);
----------------
dmgreen wrote:
> Could these include eq and ne conditions too?
Yes, updated!


================
Comment at: llvm/test/CodeGen/AArch64/vselect-ext.ll:157
 define <8 x i32> @same_zext_used_in_cmp_unsigned_pred_and_select_v8i32_2(<8 x i16> %a) {
 ; check-label: same_zext_used_in_cmp_unsigned_pred_and_select_v8i32_2:
 ; check:       ; %bb.0:
----------------
dmgreen wrote:
> Can you remove all of these with the lower cases.
Yeah I removed those. They were added by accident :(


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120481/new/

https://reviews.llvm.org/D120481