Page MenuHomePhabricator

[ms] [llvm-ml] Introduce command-line compatibility for ml.exe and ml64.exe
ClosedPublic

Authored by epastor on Oct 23 2020, 10:48 AM.

Details

Summary

Switch to OptParser for command-line handling

Diff Detail

Event Timeline

epastor created this revision.Oct 23 2020, 10:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2020, 10:48 AM
epastor requested review of this revision.Oct 23 2020, 10:48 AM

Nice!

Do tests still run after the test file renaming to .asm? llvm/test/lit.cfg.py has config.suffixes = ['.ll', '.c', '.test', '.txt', '.s', '.mir', '.yaml'] which includes .test but not .asm. I would expect that you need to edit llvm/test/tools/llvm-ml/lit.local.cfg to have config.suffixes.add('.asm,') for the tests to keep running.

llvm/tools/llvm-ml/Opts.td
7

Do we need the non-ml-compat options for anything? If so, why support '/' prefixes for this? Why not the more usual '--' here? (see also lld/ELF/Options.td for some color on this.)

What's with the '.' syntax?

thakis requested changes to this revision.Nov 4 2020, 11:32 AM

(marking this as "request changes" to get it off my dashboard until the questions above are answered. doesn't necessarily need changes, more "request replies" :) )

This revision now requires changes to proceed.Nov 4 2020, 11:32 AM
epastor updated this revision to Diff 303162.Nov 5 2020, 10:28 AM

Fix prefixes for LLVM-specific options, and add the .asm tests to the lit config

epastor updated this revision to Diff 303173.Nov 5 2020, 10:33 AM

Rebase on HEAD

epastor updated this revision to Diff 303184.Nov 5 2020, 10:43 AM

After rebasing on parent, fix new test files for invocation style

epastor updated this revision to Diff 303201.Nov 5 2020, 11:22 AM

Fix lit.local.cfg (add vs. append)

Nice!

Do tests still run after the test file renaming to .asm? llvm/test/lit.cfg.py has config.suffixes = ['.ll', '.c', '.test', '.txt', '.s', '.mir', '.yaml'] which includes .test but not .asm. I would expect that you need to edit llvm/test/tools/llvm-ml/lit.local.cfg to have config.suffixes.add('.asm,') for the tests to keep running.

... they do now. Thanks for catching this! (They still pass.)

llvm/tools/llvm-ml/Opts.td
7

We do need it for now - it lets users select things like output filetype. (We might be able to replace that with the listing options once those are implemented, and generally I plan on working to converge towards ML.EXE's interface.) I've switched to supporting only "-" and "--" here; I'm keeping "-" available for partial compatibility with llvm-mc.

(And the "." was a typo.)

thakis accepted this revision.Nov 5 2020, 11:32 AM
This revision is now accepted and ready to land.Nov 5 2020, 11:32 AM
epastor updated this revision to Diff 307162.Nov 23 2020, 12:09 PM

Rebase on parent

epastor updated this revision to Diff 307167.Nov 23 2020, 12:14 PM

Accept /c as a flag without warning; it's our current default behavior anyway

epastor updated this revision to Diff 307186.Nov 23 2020, 1:32 PM

Disable IgnoreCase; ml.exe and ml64.exe use case-sensitive flags

epastor updated this revision to Diff 307687.Nov 25 2020, 12:35 PM

Rebase on parent

epastor updated this revision to Diff 307688.Nov 25 2020, 12:35 PM

Rebase on parent

Harbormaster completed remote builds in B80138: Diff 307688.
epastor updated this revision to Diff 308473.Nov 30 2020, 1:35 PM

Update for new test files

(Just to be sure: This is ready to go and not waiting on me, right?)

(Just to be sure: This is ready to go and not waiting on me, right?)

Yep - working on getting it in, at a not-quite-top priority at the moment. It was blocked the other day behind an intermediate change.

This revision was landed with ongoing or failed builds.Dec 1 2020, 2:43 PM
This revision was automatically updated to reflect the committed changes.