Page MenuHomePhabricator

[Constants] Represent the runtime length of a scalable vector. [IR support for SVE scalable vectors 3/4]
AbandonedPublic

Authored by paulwalker-arm on Nov 24 2016, 7:20 AM.

Details

Summary
The length of a scalable vector is unknown during compilation.  When
vectorising loops the runtime length is required to update induction
variables and thus a representation is required within the IR.

This patch introduces the 'vscale' identifier to represent the scaling
factor 'n' of a scalable vector of the form '<n x #elements x ty>'.

In use, induction variable updates for scalable vectorisation become:

  ; i += number_of_elements(<n x 4 x i32)
  %i.next = add i32 %i, mul (i32 vscale, i32 4)

Event Timeline

paulwalker-arm retitled this revision from to [Constants] Represent the runtime length of a scalable vector. [IR support for SVE scalable vectors 3/4].
paulwalker-arm updated this object.
paulwalker-arm added reviewers: hfinkel, mkuper.
paulwalker-arm added a subscriber: llvm-commits.
fhahn added a subscriber: fhahn.Nov 24 2016, 7:27 AM
deadalnix requested changes to this revision.Nov 29 2016, 1:24 PM
deadalnix edited edge metadata.
deadalnix added inline comments.
include/llvm-c/Core.h
223

This changes the enum value in the C API. Please don't. This will cause hard to track failures in user that use the C API to bind other languages.

This revision now requires changes to proceed.Nov 29 2016, 1:24 PM
mehdi_amini added inline comments.Nov 29 2016, 1:29 PM
include/llvm-c/Core.h
223

We should have a comment before the enum.

Also we could we have a unittest for this:
I have in mind something that just does:

// Check that enum values don't change
if (LLVMArgumentValueKind != 0)
  report_fatal_error("Enum changed value")
If (.... )

Or:

static_assert(LLVMArgumentValueKind != 0, "C-API Enum value changed");
rengolin edited edge metadata.Nov 30 2016, 3:51 AM

Assuming we're all happy with vscale, and all noted concerns are addressed, I don't see anything wrong with this patch.

Just reminding, this is not an approval. As we discussed on the list, any final IR change decision will be taken on the list, after extensive review by more than just the few folks that are looking at these patches.

lib/IR/Constants.cpp
1363

Ouch, ok, this is a separate thing that can be done now, independent of vscale. Please move this to a different patch and propose it on a new review on its own.

sanjoy added a subscriber: sanjoy.May 7 2017, 3:05 PM
paulwalker-arm abandoned this revision.May 11 2017, 4:22 AM