Fixes a crash ('Invalid size request on a scalable vector') in visitAlloca()
when we call this function for a scalable alloca instruction, caused
by the implicit conversion of TySize to uint64_t.
This patch changes TySize to a TypeSize as returned by getTypeAllocSize()
and ensures the allocation size is multiplied by vscale for scalable vectors.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp | ||
---|---|---|
4028 | Hi @kmclaughlin, I think the known min value needs multiplying by vscale here. You can use DAG.getVScale() to do this I think. | |
llvm/test/CodeGen/AArch64/sve-alloca.ll | ||
6 | It would be good to add some check lines for the stack allocation here too if possible? |
Can we please have a bit more detailed description for this patch? What problem this patch is trying to solve?
- Fixed AllocSize to ensure we multiply by vscale for scalable vectors
- Added CHECK lines to the test in sve-alloca.ll
llvm/test/CodeGen/AArch64/sve-alloca.ll | ||
---|---|---|
2 | Hi @kmclaughlin, it seems there are some CHECK lines missing below, because the check for the condition has disappeared. Please don't remove any CHECK lines if you keep:
in the top of the file. i.e. either don't keep this line, or don't manually edit the tests after generating. In this case, I prefer to keep all context with all CHECK lines. |
LGTM
llvm/test/CodeGen/AArch64/sve-alloca.ll | ||
---|---|---|
11–15 | I think the explicit alignment could be folded away if LLVM would know something about vscale from the vscale_range attribute (through computeKnownBits?), although we'd need to know if vscale is a power of 2, which isn't encoded by vscale_range unless min(vscale_range) == max(vscale_range). In any case, because VSCALE is currently an opaque node, we always end up with the alignment code. |
- Generated the check lines in sve-alloca.ll with update_llc_test_checks.py
- Use TySize.getFixedValue() when TySize is not scalable
Hi @kmclaughlin, I think the known min value needs multiplying by vscale here. You can use DAG.getVScale() to do this I think.