This is an archive of the discontinued LLVM Phabricator instance.

[mlir][mlir-cpu-runner] Add support for `-mattr` and `-march` flags
ClosedPublic

Authored by awarzynski on Mar 26 2023, 9:15 AM.

Details

Summary

This patch adds support for -mattr and -march in mlir-cpu-runner.
With this change, one should be able to consistently use mlir-cpu-runner
for MLIR's integration tests (instead of e.g. resorting to lli when some
additional flags are needed). This is demonstrated in
concatenate_dim_1.mlir.

In order to support the new flags, this patch makes sure that
MLIR's ExecutionEngine/JITRunner (that mlir-cpu-runner is built on top of):

  • takes into account the new command line flags when creating TargetMachine,
  • avoids recreating TargetMachine if one is already available,
  • creates LLVM's DataLayout based on the previously configured TargetMachine.

This is necessary in order to make sure that the command line
configuration is propagated correctly to the backend code generator.

A few additional updates are made in order to facilitate this change,
including support for debug dumps from JITRunner.

Diff Detail

Event Timeline

awarzynski created this revision.Mar 26 2023, 9:15 AM
Herald added a project: Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Mar 26 2023, 9:15 AM

Fix Toy examples builds

Fix verify-flags.mlir

awarzynski retitled this revision from [mlir-cpu-runner] Add support for `-mattr` and `-march` flags to [mlir][mlir-cpu-runner] Add support for `-mattr` and `-march` flags.Mar 27 2023, 1:17 PM
c-rhodes added inline comments.Mar 28 2023, 2:11 AM
mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
190

nit: layout

mlir/lib/ExecutionEngine/ExecutionEngine.cpp
230

nit: drop braces on single-statement ifs? (and below)

246

I think the previous error message was fine

279–281

what happens to the error message from createDefaultTargetMachine here?

mlir/lib/ExecutionEngine/JitRunner.cpp
408

nit: unrelated change

mlir/test/Integration/Dialect/SparseTensor/CPU/concatenate_dim_1.mlir
15

this test has been removed?

mlir/test/mlir-cpu-runner/verify-flags.mlir
1

Please add a test for march as well

awarzynski marked 3 inline comments as done.Mar 28 2023, 3:55 AM

Thanks for the review @c-rhodes , sending updates shortly.

mlir/lib/ExecutionEngine/ExecutionEngine.cpp
279–281

Good catch, this should be propagated.

mlir/test/Integration/Dialect/SparseTensor/CPU/concatenate_dim_1.mlir
15

this test has been removed?

Yes, @aartbik and I discussed this offline and feel that this approach makes more sense (and this patch enables that).

Note that the 3rd and 4th RUN lines are exercising similar codepaths when ENABLE_VLA is false. Since these tests are expensive to run (and there's quite a few of them), it makes a lot of sense to reduce duplication. The solution is to replace 2 RUN lines with 1 in which the vectorizer will:

  • generate regular fixed-vector width code when ENABLE_VLA is Off
  • generate scalable SVE code when ENABLE_VLA is On.

Does it make sense?

mlir/test/mlir-cpu-runner/verify-flags.mlir
1

Good point.

For this to work, (i.e. for mlir-cpu-runner to fail in a sane way when using -march=<invalid-val>), I need to fix how errors are handled in JitRunner.cpp (just a wee heads-up)

Address Cullen's comments

In order to add a test for -march, I had to fix how errors are handled in JitRunner.cpp. Other changes should be self-explenatory.

c-rhodes accepted this revision.Mar 28 2023, 6:45 AM

LGTM, cheers

mlir/test/Integration/Dialect/SparseTensor/CPU/concatenate_dim_1.mlir
15

this test has been removed?

Yes, @aartbik and I discussed this offline and feel that this approach makes more sense (and this patch enables that).

Note that the 3rd and 4th RUN lines are exercising similar codepaths when ENABLE_VLA is false. Since these tests are expensive to run (and there's quite a few of them), it makes a lot of sense to reduce duplication. The solution is to replace 2 RUN lines with 1 in which the vectorizer will:

  • generate regular fixed-vector width code when ENABLE_VLA is Off
  • generate scalable SVE code when ENABLE_VLA is On.

Does it make sense?

It does, thanks for clarifying.

This revision is now accepted and ready to land.Mar 28 2023, 6:45 AM
Matt added a subscriber: Matt.Mar 28 2023, 12:29 PM
mehdi_amini added inline comments.Mar 28 2023, 2:28 PM
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
240

Can we update this code to use JITTargetMachineBuilder (and JITTargetMachineBuilder::detectHost) instead of doing it manually here?

272

I have some concerns that your change here is going against the direction hinted by this TODO, what is the future plan about this? (your update to the TODO does not carve a path out)

awarzynski added inline comments.Mar 29 2023, 7:19 AM
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
240

Good point, this is basically re-implementing detectHost. I will delete it altogether.

272

This is a bit tricky :)

A triple could be defined in 3 different places (perhaps more?):

  1. through methods like JITTargetMachineBuilder::detectHost/lvm::sys::getHostCPUName (the default value),
  2. through command line arguments, e.g. -march, -target, -triple (the user-specified value),
  3. inside the input LLVM module (the "input" defined value or via MLIR conversion/translation).

Prior to this change, only Option 1. was supported. With this change, mlir-cpu-runner will also support Option 2. This FIXME refers to Option 3. I replaced it with a TODO, because FIXME suggests that something is broken, but IMO that's not the case. It just happens that Options 2. and 3. are not supported ATM. My goal in this patch is to add support for Option 2 while keeping the path open for Option 3..

I said that this is tricky, because with 3 sources of the same information, one has to decide what takes precedence and then make sure that that's what's actually implemented. Clang and Flang have similar problem and approach it by ignoring Option 3 (tested on MacOS):

$ cat file.ll
target triple = "some-triple-different-to-host-triple"

define void @foo() {
  ret void
}
$ clang  -S file.ll
warning: overriding the module target triple with arm64-apple-macosx13.0.0 [-Woverride-module]
1 warning generated.

Sorry, this is a bit longer than I expected. The point that I'm trying to make is that:

  • This patch only adds support for Option 2,
  • Option 3. remains a TODO.

Is this change going to make Option 3. harder? I don't think so, but we'd need a motivating example to better understand what might be needed. We had a somewhat similar issue in Flang [1], and that wasn't too hard to support that (though that was specifically about DataLayout).

[1] https://github.com/llvm/llvm-project/issues/57230

Delete createDefaultTargetMachine

Fix build failures in the Tay examples

The outstanding pre-commit failure is unrelated to this change:

********************
Failed Tests (1):
  Flang :: Driver/omp-frontend-forwarding.f90
 
Testing Time: 15.32s
  Unsupported      :   48
  Passed           : 2019
  Expectedly Failed:   15
  Failed           :    1
mehdi_amini accepted this revision.Mar 30 2023, 1:55 PM
mehdi_amini added inline comments.
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
272

Is this change going to make Option 3. harder? I don't think so, but we'd need a motivating example to better understand what might be needed

Basically that was the main thing I wanted to understand, I'm not sure I totally have wrapped my head entirely about this, but I'm willing to trust your judgement on this :)

This revision was landed with ongoing or failed builds.Mar 31 2023, 12:34 AM
This revision was automatically updated to reflect the committed changes.

Btw, this patch refines how diagnostics are propagated, so you might see this:

$ ninja check-mlir
[35/36] Running the MLIR regression tests
Symbols not found: [ llvm_orc_registerEHFrameSectionWrapper ]

Testing Time: 5.63s
  Unsupported      :  190
  Passed           : 1813
  Expectedly Failed:    1

This comes from mlir-cpu-runner -host-supports-jit:

bin/mlir-cpu-runner --host-supports-jit
false
Symbols not found: [ llvm_orc_registerEHFrameSectionWrapper ]

It's a genuine diagnostic that wasn't reported before, but the error itself is bogus. I'm trying to fix it here: https://reviews.llvm.org/D146935 (however, that made me discover https://github.com/llvm/llvm-project/issues/61856).