fhahn marked an inline comment as done. fhahn added a comment. Thanks for all the feedback John!
================ Comment at: clang/lib/CodeGen/CGExpr.cpp:3787 + ColIdx->getType()->getScalarSizeInBits()); + llvm::Type *IntTy = llvm::IntegerType::get(getLLVMContext(), MaxWidth); + RowIdx = Builder.CreateZExt(RowIdx, IntTy); ---------------- rjmccall wrote: > 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. > It's UB if it's actually negative, right? So the sext really shouldn't be a > problem. Yes exactly. At the C/C++ level, the individual row/column expressions are guaranteed to be `>= 0`. In the IR we emit, we compute the index in a vector and we know that the computed index must be `>= 0`, which is slightly weaker. It might be worth checking how to encode the fact that the extension sources must be `>= 0`, if it becomes 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
