This is an archive of the discontinued LLVM Phabricator instance.

[Matrix] Overload stride arg in matrix.columnwise.load/store.
ClosedPublic

Authored by fhahn on Aug 3 2021, 6:48 AM.

Details

Summary

This patch adjusts the intrinsics definition of
llvm.matrix.column.major.load and llvm.matrix.column.major.store to
allow overloading the type of the stride. The bitwidth of the stride is
used to perform the offset computation.

This fixes a crash when using builtin_matrix_column_major_load or
builtin_matrix_column_major_store on 32 bit platforms. The stride argument
of the builtins are defined as size_t, which is 32 bits wide on 32 bit
platforms.

Note that we still perform offset computations with 64 bit width on 32
bit platforms for accesses that do not take a user-specified stride.
This can be fixed separately.

Fixes PR51304.

Diff Detail

Event Timeline

fhahn created this revision.Aug 3 2021, 6:48 AM
fhahn requested review of this revision.Aug 3 2021, 6:48 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 3 2021, 6:48 AM
erichkeane accepted this revision.Aug 3 2021, 6:55 AM

This looks fine to me, and seems to fix my problem, thanks! I didn't spot anything obvious,and proof-read the LangRef and think it is all fine, but am not really the expert here, so please don't commit without the others having a day or two to comment.

This revision is now accepted and ready to land.Aug 3 2021, 6:55 AM
This revision was landed with ongoing or failed builds.Aug 12 2021, 3:07 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Aug 12 2021, 3:09 AM

This looks fine to me, and seems to fix my problem, thanks! I didn't spot anything obvious,and proof-read the LangRef and think it is all fine, but am not really the expert here, so please don't commit without the others having a day or two to comment.

Thanks! I landed the change as there have not been any further comments in a while.

akuegel added a subscriber: akuegel.EditedAug 12 2021, 4:47 AM

It seems this patch caused a test failure in MLIR:
test/Target/LLVMIR/llvmir-intrinsics.mlir

Revert to unbreak bots (like this one : https://lab.llvm.org/buildbot/#/builders/13/builds/10930 )

Looks like this should be a pretty trivial test update at least.

fhahn added a comment.Aug 13 2021, 1:16 AM

Revert to unbreak bots (like this one : https://lab.llvm.org/buildbot/#/builders/13/builds/10930 )

Looks like this should be a pretty trivial test update at least.

Thanks for the heads-up! Recommitted with an updated MLIR test: f999312872b8