[PATCH] D97480: [RISCV] Support inline asm for vector instructions.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:5690
+    case 'v':
+      if (TRI->isTypeLegalForClass(RISCV::VMRegClass, VT.SimpleTy))
+        return std::make_pair(0U, &RISCV::VMRegClass);
----------------
I might be missing something: could `VM` be folded into the loop below?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6218
+      // PartVT first.
+      if (ValueEltVT != PartEltVT) {
+        unsigned Count = ValueVTBitSize / PartEltVT.getSizeInBits();
----------------
HsiangKai wrote:
> frasercrmck wrote:
> > HsiangKai wrote:
> > > frasercrmck wrote:
> > > > In what situations are the element types not equal? Does the PartVT come from the first type in the RC?
> > > Yes, the PartVT comes from the first type in the RC. Another way to handle it is to search the corresponding type in the RC in GetRegistersForValue(). The problem comes from we reuse VR register class for fractional LMUL types.
> > Thanks! (Un)Luckily I've seen the "first type" issue before: I wish there was a better way.
> @rogfer01 provided another way to deal with the issue in GetRegistersForValue() in their repository.
> 
> He searched the types in the register class for the value type. Do you think it is better to do in this way or provide target hooks to deal with it?
> 
> ```
> @@ -7906,7 +7906,20 @@ static void GetRegistersForValue(SelectionDAG &DAG, const SDLoc &
>    // Get the actual register value type.  This is important, because the user
>    // may have asked for (e.g.) the AX register in i32 type.  We need to
>    // remember that AX is actually i16 to get the right extension.
> -  const MVT RegVT = *TRI.legalclasstypes_begin(*RC);
> +  MVT RegVT = *TRI.legalclasstypes_begin(*RC);
> +  // Try harder for vectors just in case sizes don't match.
> +  // FIXME: This is caused by having nxv1i1 in the same register class as
> +  // nxv1i64 in EPI.
> +  if (RegVT.isVector() && RefOpInfo.ConstraintVT.isVector() &&
> +      RegVT.getSizeInBits() != RefOpInfo.ConstraintVT.getSizeInBits()) {
> +    auto E = TRI.legalclasstypes_end(*RC);
> +    auto I = std::find_if(TRI.legalclasstypes_begin(*RC), E,
> +                          [&RefOpInfo](const MVT::SimpleValueType &VTy) {
> +                            return MVT(VTy) == RefOpInfo.ConstraintVT;
> +                          });
> +    if (I != E)
> +      RegVT = *I;
> +  }
> ```
It's not really specific to vectors, so if we were to go down that route I'd probably expect us to have to offer it up to everything. If I remember correctly, I once had i8,i16,i32,v2i8,v4i8,v2i16 (maybe others) in the same "scalar" register class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97480