[PATCH] D112599: [RISCV] Fix vm operand constraint to fit GCC's behavior

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9847
+          assert(Count != 0 && "The number of element should not be zero.");
+          EVT LargerEltTypeVT =
+              EVT::getVectorVT(Context, ValueEltVT, Count, /*IsScalable=*/true);
----------------
The name `LargerEltTypeVT`, to me anyway, implies it's a vector type with a larger element type when in fact it's a vector with the same element type. Maybe something like `WideValueVT`?

EDIT: Though that said, in `joinRegisterPartsIntoValue` you're keeping the name `SameEltTypeVT`. I'd prefer consistency in whatever we pick.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9851
+                            DAG.getUNDEF(LargerEltTypeVT), Val,
+                            DAG.getConstant(0, DL, Subtarget.getXLenVT()));
+        }
----------------
I realise you're copying this from the existing code but shouldn't we be using `DAG.getVectorIdxConstant(0, DL)` here?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:9854
+        Val = DAG.getNode(ISD::BITCAST, DL, PartVT, Val);
+      } else
+        Val =
----------------
I'm wondering if we should place braces around this, given the `else` is a multi-line statement. I think we do that elsewhere.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:529
 
-defvar VMaskVTs = [vbool64_t, vbool32_t, vbool16_t, vbool8_t,
-                   vbool4_t, vbool2_t, vbool1_t];
+defvar VMaskVTs = [vbool1_t, vbool64_t, vbool32_t, vbool16_t, vbool8_t,
+                   vbool4_t, vbool2_t];
----------------
Good catch! I'd prefer to properly order them as in `VM` below though. Which begs the question: why isn't `VM` using `VMaskVTs`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112599