[PATCH] D102073: [TargetLowering] Legalize "vscale x 1" types in more cases

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

In D102073#2753558 <https://reviews.llvm.org/D102073#2753558>, @sdesmalen wrote:

> Hi @frasercrmck, thanks for working on this. We're currently investigating legalization of <vscale x 1 x eltty> vectors for SVE, so we'll be able to add some tests for the widening case.

Hi @sdesmalen, thanks for getting in touch. I've just updated the patch to add some initial widening support for AArch64. Let me know what you think. It's very basic right now as I've found that we can't yet widen the subvector operations.



================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:997
+    if (LK.first == TypeExpandInteger) {
+      if (VT.getVectorElementCount() == ElementCount::getScalable(1))
+        report_fatal_error("Cannot legalize this scalable vector");
----------------
sdesmalen wrote:
> I found that when widening <vscale x 1 x eltty>, it shouldn't go down this code-path and hit the fatal error, but rather continue down the code path below (so that it will actually try to widening to <vscale x 2 x eltty>, <vscale x 4 x eltty>, etc. until it has found a legal one.
> 
> That said, without a way to test this having the fatal_error is fine for now, especially since we'll soon share some patches that implements the widening of these types with corresponding tests. vscale x 1 types are never legal for SVE and always need widening.
Thanks for the input. I was wondering that whether widening would be better, yeah. I think holding off until your patch sounds reasonable.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:1515
   LegalizeTypeAction TA = getTypeAction(Context, VT);
-  if (EltCnt.getKnownMinValue() != 1 &&
+  if ((EltCnt.isScalable() || EltCnt.getFixedValue() != 1) &&
       (TA == TypeWidenVector || TA == TypePromoteInteger)) {
----------------
sdesmalen wrote:
> Is this equivalent to `EltCnt.isVector()` ? (I assume we can assert that a minimum value of 0 is not allowed)
I think it's more strictly equivalent to `!EltCnt.isScalar()` which I was about to update to use. I think that's marginally more readable as people may wonder why we're checking `isVector` in what's obviously a vector method. What do you think?


================
Comment at: llvm/test/CodeGen/RISCV/rvv/legalize-scalable-vectortype.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -mattr=+experimental-v -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -mtriple=riscv64 -mattr=+experimental-v -verify-machineinstrs < %s | FileCheck %s
----------------
frasercrmck wrote:
> craig.topper wrote:
> > frasercrmck wrote:
> > > craig.topper wrote:
> > > > These tests don't cover widening do they?
> > > True, they don't. To be honest I was specifically targeting the promotion but the code looks like it should handle widening okay. I'm not sure we have an RVV case where we have to widen an illegal `<vscale x 1 x ty>` type to a legal one.
> > I can't remember if the caller can handle widening scalable vectors properly. I fought with this code before when we tried to use types like <vscale x 6 x ty> for x6 segment load/stores.
> > 
> > Can the vectorize create these <vscale x 4 x i5> types that need promotion?
> Yeah I wouldn't be surprised. Since this patch only affects `vscale x 1` types, any existing issues in widening scalable vectors are already out in the open. I think without proper tests, ot might be best to remove the claim of "allowing" widening from this patch. At least we can say it now treats `vscale x 1` more similarly to the others.
>  
> I haven't looked too deeply but I believe these `i5`s can appear during switch generation or switch case optimizations. I don't know about the loop vectorizer, but our WFV vectorizer will happily vectorize these.
I've added some initial widening tests for AArch64.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102073