Page MenuHomePhabricator

[MLIR][GPU] Update SerializeToHsaco to match downstream
AbandonedPublic

Authored by krzysz00 on Oct 27 2021, 3:21 PM.

Details

Summary

[MLIR][GPU] Run generic LLVM optimizations when serializing (on AMD)

  • Adds hooks that allow SerializeTo* passes to arbitrarily transform

the produced LLVM Module before it is passed to the code generation
passes.

  • Uses these hooks within the SerializeToHsaco pass in order to run

LLVM optimizations and to set the optimization level on the
TargetMachine.

  • Adds an optLevel parameter to SerializeToHsaco

Future work may include moving much of what's been added to
SerializeToHsaco to SerializeToBlob, but that would require
confirmation from the NVVM backend maintainers that it would be
appropriate to do so.

[MLIR][AMDGPU] Link device libraries where needed

  • The ROCm library path is now computed at runtime instead of relying

on a compile-time default (except as a fallback)

  • SerializeToHsaco no longer depends on HIP, as it no longer does

chipset autodetection (which wasn't being used anyway)

  • A --rocm-path option has been added to allow the user to override

the ROCm path, in addition to the typical ROCM_PATH environment
variable

Diff Detail

Event Timeline

krzysz00 created this revision.Oct 27 2021, 3:21 PM
krzysz00 requested review of this revision.Oct 27 2021, 3:21 PM
krzysz00 updated this revision to Diff 384120.Nov 2 2021, 8:31 AM
  • Restore the --gpu-to-hsaco option because it's used in integration tests. Update
krzysz00 updated this revision to Diff 385938.Nov 9 2021, 12:33 PM
  • Restore the --gpu-to-hsaco option because it's used in integration tests. Update
  • Pass rocm path to integration tests

Nit: "Update SerializeToHsaco to match downstream" isn't a very clear / descriptive commit title here.

Also the description seems like an aggregate of changes, see here for a more explicit guideline: https://mlir.llvm.org/getting_started/Contributing/#commit-messages (and https://chris.beams.io/posts/git-commit/ ).

If you can't describe the changes easily, that may be an indication that there are many changes that would better be split in separate changes.

mehdi_amini added inline comments.Nov 16 2021, 4:48 PM
mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
73

The default copy constructor wouldn't copy all these already?

If you can't describe the changes easily, that may be an indication that there are many changes that would better be split in separate changes.

Yeah, I think I can split this into multiple changesets. The catch will be splitting in such a way that each changeset doesn't break the build. Fortunately, our repo has the history of the various changes, though I can't directly yank the patches out.

I'll find a way to withdraw this and send in smaller parts - can I add you as a reviewer on each of those?

mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp
73

It didn't seem to (I end up with highly negative optLevel values) but maybe that was some other issue.