[PATCH] D112599: [RISCV] Fix vm operand constraint to fit GCC's behavior
Fraser Cormack via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 27 03:32:34 PDT 2021
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
More information about the llvm-commits
mailing list