rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land.
LGTM, thanks. ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:3787 + ColIdx->getType()->getScalarSizeInBits()); + llvm::Type *IntTy = llvm::IntegerType::get(getLLVMContext(), MaxWidth); + RowIdx = Builder.CreateZExt(RowIdx, IntTy); ---------------- fhahn wrote: > rjmccall wrote: > > fhahn wrote: > > > rjmccall wrote: > > > > You should be able to assert that these have been coerced to the right > > > > type by Sema (probably size_t or ptrdiff_t or something). > > > Hm I don't think that's currently happening. Is that required? It seems a > > > bit unfortunate if we would have to force to wider bit-widths than > > > necessary for the given index expressions. > > Can you explain what cost you're worried about? I don't understand what > > the benefit of using a smaller IR type for the indexes is, and usually it's > > better to have stronger invariants coming out of Sema. > I now updated the code to force the index types to size_t in Sema and remove > the zext generation here. If we have signed indices, we now generate sexts, > which may require additional instructions down the road., which we would not > require if we perform the math on the max bit width of the operands. But > looking back now, I am not really sure if that would actually be safe in all > cases. > > We should probably just try to narrow the index types based on the number of > vector elements as an IR optimization, if it becomes a problem. It's UB if it's actually negative, right? So the `sext` really shouldn't be a problem. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76791/new/ https://reviews.llvm.org/D76791 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
