jrtc27 added inline comments.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:50
+ Boolean,
+ SignInteger,
+ UnsignedInteger,
----------------
Signed
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:58-62
+ bool IsPointer = false;
+ // IsConstant indices are "int", but have the constant expression.
+ bool IsImmediate = false;
+ // const qualifier.
+ bool IsConstant = false;
----------------
This isn't expressive enough for the grammar you defined. `PCPCec` is supposed
to give `const i8 * const i8 *`, whereas this will interpret it as `const i8
*`. Given such types are presumably not needed you need to tighten the rules of
your grammar.
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:127
+
+enum RISCV_Extension : uint8_t {
+ Basic = 0,
----------------
No underscores in names
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:173
+ StringRef getIRName() const { return IRName; }
+ uint8_t getRISCV_Extensions() const { return RISCV_Extensions; }
+
----------------
No underscores in names
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:354-356
+ }
+
+ switch (ScalarType) {
----------------
Combine the two switch statements
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:419
+ ClangBuiltinStr = "__rvv_";
+ if (isBoolean()) {
+ ClangBuiltinStr += "bool" + utostr(64 / Scale.getValue()) + "_t";
----------------
Combine this with the switch
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:441
+ assert(isValid() && "RVVType is invalid");
+ assert(ScalarType != ScalarTypeKind::Invalid && "ScalarType is invalid");
+ switch (ScalarType) {
----------------
Combine this with the switch
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:452-467
+ default:
+ break;
+ }
+
+ if (IsConstant)
+ Str += "const ";
+
----------------
This should be able to be tidied up so there's only one switch
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:627
+ case 'W':
+ assert(isVector() && "'W' type transformer cannot be used on vectors");
+ ElementBitwidth *= 2;
----------------
This looks wrong
================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:259-260
+
+LMULType &LMULType::operator*=(unsigned RHS) {
+ this->Log2LMUL = this->Log2LMUL + RHS;
+ return *this;
----------------
khchen wrote:
> craig.topper wrote:
> > jrtc27 wrote:
> > > That's not how multiplication works. This is exponentiation.
> > > Multiplication would be `Log2LMul + log2(RHS)`. Please don't abuse
> > > operators like this.
> > This seems like it must be broken, but since we don't do widening or
> > narrowing in this patch we didn't notice?
> Yes, thanks for point out. In my original plan is fixing that in followup
> patches.
> I also add more bug fixes into this patch.
Probably worth adding an an `assert(isPowerOf2_32(RHS));` too
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95016/new/
https://reviews.llvm.org/D95016
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
