Page MenuHomePhabricator

[Matrix] Add remark propagation along the inlined-at chain.
ClosedPublic

Authored by fhahn on Jan 28 2020, 5:52 PM.

Details

Summary

This patch adds support for propagating matrix expressions along the
inlined-at chain and emitting remarks at the traversed function scopes.

To motivate this new behavior, consider the example below. Without the
remark 'up-leveling', we would only get remarks in load.h and store.h,
but we cannot generate a remark describing the full expression in
toplevel.cpp, which is the place where the user has the best chance of
spotting/fixing potential problems.

With this patch, we generate a remark for the load in load.h, one for
the store in store.h and one for the complete expression in
toplevel.cpp. For a bigger example, please see remarks-inlining.ll.

load.h:
template <typename Ty, unsigned R, unsigned C> Matrix<Ty, R, C> load(Ty *Ptr) {
  Matrix<Ty, R, C> Result;
  Result.value = *reinterpret_cast <typename Matrix<Ty, R, C>::matrix_t *>(Ptr);
  return Result;
}

store.h:
template <typename Ty, unsigned R, unsigned C> void store(Matrix<Ty, R, C> M1, Ty *Ptr) {
   *reinterpret_cast<typename decltype(M1)::matrix_t *>(Ptr) = M1.value;
}

toplevel.cpp
void test(double *A, double *B, double *C) {
  store(add(load<double, 3, 5>(A), load<double, 3, 5>(B)), C);
}

For a given function, we traverse the inlined-at chain for each
matrix instruction (= instructions with shape information). We collect
the matrix instructions in each DISubprogram we visit. This produces a
mapping of DISubprogram -> (List of matrix instructions visible in the
subpogram). We then generate remarks using the list of instructions for
each subprogram. This allows surfacing the remarks at a level useful to
users.
Please note that the current approach may create a lot of extra remarks.
Additional heuristics to cut-off the traversal can be implemented in the
future. For example, it might make sense to stop 'up-leveling' once all
matrix instructions are at the same debug location.

Diff Detail

Event Timeline

fhahn created this revision.Jan 28 2020, 5:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2020, 5:52 PM

Unit tests: fail. 62274 tests passed, 1 failed and 827 were skipped.

failed: Clang.CodeGenOpenCL/amdgpu-features.cl

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

On the description: I would first explain what you do and then how to do, i.e. have "To motivate" and the example before the paragprah "For a given function, we traverse".

llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
960

I am assuming these values that falls into a given DISubprogram? More explanation would be good here and probably a bit more specific name than ExprSet.

1222

Explain Ops in the comment.

1297

Mapping is usually not a good name ;)

fhahn updated this revision to Diff 248930.Mar 7 2020, 7:07 AM

Add additional comments, fix some variable names.

On the description: I would first explain what you do and then how to do, i.e. have "To motivate" and the example before the paragprah "For a given function, we traverse".

Sounds good, thanks. I'll re-order the paragraphs.

fhahn marked 5 inline comments as done.Mar 7 2020, 7:09 AM
fhahn added inline comments.
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
960

Yes, I've renamed it to ExprsInSubprogram and updated the comment.

1222

Also renamed Ops to ExprsInSubprogram.

1297

Done, renamed to Subprog2Exprs.

fhahn edited the summary of this revision. (Show Details)Mar 7 2020, 7:10 AM
anemet accepted this revision.Mar 11 2020, 8:55 AM

In the description when you say:

We then generate remarks using the list of instructions for
each subprogram. This allows surfacing the remarks at a level useful to
users.

I would make it explicit that here subprograms is meant to include its own subprograms recursively. E.g. using the example for the subprogram test this includes load and store inlined functions.

LGTM with these.

llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
1207

Also explain why we need leaves and what that means for sharing. Again an example would be useful.

1226

returning

This revision is now accepted and ready to land.Mar 11 2020, 8:55 AM
fhahn marked 2 inline comments as done.Mar 11 2020, 10:41 AM

In the description when you say:

We then generate remarks using the list of instructions for
each subprogram. This allows surfacing the remarks at a level useful to
users.

I would make it explicit that here subprograms is meant to include its own subprograms recursively. E.g. using the example for the subprogram test this includes load and store inlined functions.

LGTM with these.

Thanks, will do!

This revision was automatically updated to reflect the committed changes.