This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Fix TypeSize->uint64_t implicit conversion in visitAlloca()
ClosedPublic

Authored by kmclaughlin on Jan 27 2022, 7:59 AM.

Details

Summary

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.

Diff Detail

Event Timeline

kmclaughlin created this revision.Jan 27 2022, 7:59 AM
kmclaughlin requested review of this revision.Jan 27 2022, 7:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2022, 7:59 AM
david-arm added inline comments.Jan 27 2022, 8:22 AM
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?

kmclaughlin marked 2 inline comments as done.
kmclaughlin edited the summary of this revision. (Show Details)
  • Fixed AllocSize to ensure we multiply by vscale for scalable vectors
  • Added CHECK lines to the test in sve-alloca.ll
sdesmalen added inline comments.Jan 28 2022, 4:24 AM
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:

; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py

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.

sdesmalen accepted this revision.Jan 28 2022, 5:52 AM

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.

This revision is now accepted and ready to land.Jan 28 2022, 5:52 AM
  • Generated the check lines in sve-alloca.ll with update_llc_test_checks.py
  • Use TySize.getFixedValue() when TySize is not scalable
This revision was landed with ongoing or failed builds.Jan 31 2022, 6:38 AM
This revision was automatically updated to reflect the committed changes.