This is an archive of the discontinued LLVM Phabricator instance.

[Matrix] Emit assumption that matrix indices are valid.
ClosedPublic

Authored by fhahn on May 14 2021, 4:09 AM.

Details

Summary

The matrix extension requires the indices for matrix subscript
expression to be valid and it is UB otherwise.

extract/insertelement produce poison if the index is invalid, which
limits the optimizer to not be bale to scalarize load/extract pairs for
example, which causes very suboptimal code to be generated when using
matrix subscript expressions with variable indices for large matrixes.

This patch updates IRGen to emit assumes to for index expression to
convey the information that the index must be valid.

This also adjusts the order in which operations are emitted slightly, so
indices & assumes are added before the load of the matrix value.

Diff Detail

Event Timeline

fhahn created this revision.May 14 2021, 4:09 AM
fhahn requested review of this revision.May 14 2021, 4:09 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 14 2021, 4:09 AM
fhahn updated this revision to Diff 345702.May 16 2021, 5:14 AM

Thanks for taking a look! I added a phase ordering test and updated the pipeline tests as well.

fhahn updated this revision to Diff 345703.May 16 2021, 5:49 AM

Undo unrelated changes.

xbolva00 added inline comments.
llvm/include/llvm/IR/MatrixBuilder.h
242

Prefer early exit?

xbolva00 added inline comments.May 16 2021, 9:21 AM
llvm/include/llvm/IR/MatrixBuilder.h
245

B.CreateAssumption(Cmp) may work well as well? and shorter.

Do you want to generate these even at -O0?

llvm/include/llvm/IR/MatrixBuilder.h
242

Should this do something special if the index is statically out of bounds?

fhahn updated this revision to Diff 348210.May 27 2021, 3:48 AM

Thanks for the comments!

Updates:

  • Only emit assumptions for optimized builds; the assumes are not helpful for unoptimized builds
  • Use builder::CreateAssumption instead of manually constructing assume calls
  • Add assert that condition must be true, if simplified to a constant.
fhahn marked an inline comment as done.May 27 2021, 3:50 AM
fhahn added inline comments.
llvm/include/llvm/IR/MatrixBuilder.h
242

I think this cause should never happen and if it does it means earlier checks have been missed. I added an assert.

245

Updated, thanks!

fhahn updated this revision to Diff 348212.May 27 2021, 3:51 AM
fhahn marked an inline comment as done.

Fix clang-tidy warnings.

fhahn updated this revision to Diff 348357.May 27 2021, 12:40 PM

Fix failing clang/test/CodeGenObjC/matrix-type-operators.m.

Just a couple of nits here, basically see how much we can put in the 'cheap to check' branch.

clang/lib/CodeGen/CGExpr.cpp
1944

I think the linter here is right, we require const auto * here.

1946

Is it worth putting the MatrixBuilder line inside of this branch too? Perhaps all the added code?

2091

Same here on both.

fhahn updated this revision to Diff 352108.Jun 15 2021, 5:52 AM

Just a couple of nits here, basically see how much we can put in the 'cheap to check' branch.

Thanks! I moved the suggested bits inside the then branch.

erichkeane accepted this revision.Jun 15 2021, 6:20 AM
This revision is now accepted and ready to land.Jun 15 2021, 6:20 AM
fhahn updated this revision to Diff 365959.Aug 12 2021, 4:05 AM

rebased after latest changes

fhahn added a comment.Sep 22 2021, 2:48 AM

All required changes to make use of this have recently landed or are ready to land. So I am going to commit this momentarily.

This revision was landed with ongoing or failed builds.Sep 22 2021, 4:28 AM
This revision was automatically updated to reflect the committed changes.