[PATCH] D150926: [RISCV] Support LMUL!=1 for __attribute__((riscv_rvv_vector_bits(N)))
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/Sema/SemaType.cpp:8338 // The attribute vector size must match -mrvv-vector-bits. - if (VecSize != VScale->first * MinElts * EltSize) { + unsigned ExpectedSize = VScale->first * MinElts * EltSize; + if (VecSize != ExpectedSize) { craig.topper wrote: > aaron.ballman wrote: > > Random thought I didn't think to ask earlier: is it possible for this > > multiplication to overflow (if so, we should add test coverage to make sure > > we don't do bad things)? > The largest value for -mrvv-vector-bits is 65536 which makes VScale->first > 65536/64 == 1024. MinElts and EltSize should be small enough that it won't > overflow. > > I'm not sure if there's any protection if someone bypasses the driver with > `-Xclang -mvscale-min=` and uses a larger value. > The largest value for -mrvv-vector-bits is 65536 which makes VScale->first > 65536/64 == 1024. MinElts and EltSize should be small enough that it won't > overflow. Fantastic, thank you! > I'm not sure if there's any protection if someone bypasses the driver with > -Xclang -mvscale-min= and uses a larger value. IMO, using cc1 options is compiler YOLO-mode, so I don't mind if this has no protection. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150926/new/ https://reviews.llvm.org/D150926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150926: [RISCV] Support LMUL!=1 for __attribute__((riscv_rvv_vector_bits(N)))
craig.topper added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:8338 // The attribute vector size must match -mrvv-vector-bits. - if (VecSize != VScale->first * MinElts * EltSize) { + unsigned ExpectedSize = VScale->first * MinElts * EltSize; + if (VecSize != ExpectedSize) { aaron.ballman wrote: > Random thought I didn't think to ask earlier: is it possible for this > multiplication to overflow (if so, we should add test coverage to make sure > we don't do bad things)? The largest value for -mrvv-vector-bits is 65536 which makes VScale->first 65536/64 == 1024. MinElts and EltSize should be small enough that it won't overflow. I'm not sure if there's any protection if someone bypasses the driver with `-Xclang -mvscale-min=` and uses a larger value. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150926/new/ https://reviews.llvm.org/D150926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150926: [RISCV] Support LMUL!=1 for __attribute__((riscv_rvv_vector_bits(N)))
aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:2350 + +``vbool*_t`` types are not supported at this time. }]; Do you have test coverage that demonstrates this? Comment at: clang/lib/AST/Type.cpp:2437 switch (BT->getKind()) { // FIXME: Support more than LMUL 1. #define RVV_VECTOR_TYPE(Name, Id, SingletonId, NumEls, ElBits, NF, IsSigned, IsFP) \ FIXME is now stale and can be removed? Comment at: clang/lib/Sema/SemaType.cpp:8338 // The attribute vector size must match -mrvv-vector-bits. - if (VecSize != VScale->first * MinElts * EltSize) { + unsigned ExpectedSize = VScale->first * MinElts * EltSize; + if (VecSize != ExpectedSize) { Random thought I didn't think to ask earlier: is it possible for this multiplication to overflow (if so, we should add test coverage to make sure we don't do bad things)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150926/new/ https://reviews.llvm.org/D150926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150926: [RISCV] Support LMUL!=1 for __attribute__((riscv_rvv_vector_bits(N)))
craig.topper added a comment. Ping * 3 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150926/new/ https://reviews.llvm.org/D150926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150926: [RISCV] Support LMUL!=1 for __attribute__((riscv_rvv_vector_bits(N)))
craig.topper added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150926/new/ https://reviews.llvm.org/D150926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150926: [RISCV] Support LMUL!=1 for __attribute__((riscv_rvv_vector_bits(N)))
craig.topper added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150926/new/ https://reviews.llvm.org/D150926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150926: [RISCV] Support LMUL!=1 for __attribute__((riscv_rvv_vector_bits(N)))
asb added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:2347 -Only ``*m1_t`` (LMUL=1) types are supported at this time. +For types where LMUL!=1, ``__riscv_v_fixed_vlen`` needs to be scaled by the LMULo +of the type before passing to the attribute. LMULo -> LMUL Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150926/new/ https://reviews.llvm.org/D150926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits