This is an archive of the discontinued LLVM Phabricator instance.

[ms] [llvm-ml] Improve data support, adding names and complex initializers.
ClosedPublic

Authored by epastor on Jan 22 2020, 2:03 PM.

Details

Diff Detail

Event Timeline

epastor created this revision.Jan 22 2020, 2:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2020, 2:03 PM

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

epastor updated this revision to Diff 244255.Feb 12 2020, 1:06 PM

Add support for complex real-valued initializers

epastor updated this revision to Diff 244888.Feb 16 2020, 9:26 AM

Correct for casing & casting changes in previous rebase

epastor updated this revision to Diff 244897.Feb 16 2020, 1:46 PM

Fix several clang-tidy and clang-format suggestions

thakis added inline comments.Feb 21 2020, 12:27 PM
llvm/lib/MC/MCParser/MasmParser.cpp
2889

What does ml do with

BYTE 2, 4, 6, 8,
BYTE 2, 4, 6, 8,

? Does it really error out?

epastor marked 2 inline comments as done.Feb 21 2020, 12:37 PM
epastor added inline comments.
llvm/lib/MC/MCParser/MasmParser.cpp
2889

Believe it or not, yes!

test.asm(2) : error A2206:missing operator in expression
thakis accepted this revision.Feb 21 2020, 12:41 PM
thakis added inline comments.
llvm/lib/MC/MCParser/MasmParser.cpp
3018

Is there no more efficient way to express this internally? If you 1000000 dup (2), do we have to write out 1M 2s?

This revision is now accepted and ready to land.Feb 21 2020, 12:41 PM
epastor marked 2 inline comments as done.Feb 21 2020, 1:00 PM
epastor added inline comments.
llvm/lib/MC/MCParser/MasmParser.cpp
3018

We could optimize the case of 1000000 dup (2) with a fill-type instruction, but I'm not sure how we would handle (e.g.) 500000 dup (2,3). Also, this is data-initialization logic, so either way the actual binary is going to contain 1M literal copies. Do you think it's worth special-casing the single-value handling internally anyway?

thakis added inline comments.Feb 21 2020, 5:28 PM
llvm/lib/MC/MCParser/MasmParser.cpp
3018

as-is is fine for now

epastor marked 3 inline comments as done.Feb 24 2020, 10:02 AM
epastor added inline comments.
llvm/lib/MC/MCParser/MasmParser.cpp
3018

Just a note for the future. Currently this means we actually make 1M copies of the APInt representing a real with value 2 in memory while compiling, and don't let go until we've processed the entire initializer list.

There are three cases here:

  1. 1000000 dup (2) - writes out 2,2,2,2,... to 1M positions.
  2. 500000 dup (2,3) - writes out 2,3,2,3,... to 1M positions.
  3. 250000 dup (2, 3 dup (1)) - writes out 2,1,1,1,2,1,1,1,... to 1M positions.

Case 1 could be optimized by using Fill logic.
Case 2 could be handled by passing a "duplication count" to the recursive call, and letting it emit results directly.

Case 3 is more of a problem, though. Arbitrarily nested duplications get hard unless you expand them in place, as the current code does... or unless you parse them into a specialized tree structure, and expand it back to actual values only as needed.

All of this is complicated by the fact that once STRUCT support is in place, we do need to be able to parse initializers and hold them in memory, not just emit them immediately.

The ideal solution would probably be a tree-based intermediate representation for initializers, with duplication nodes representing "X DUP (<child>)". It's probably worth coming back to this eventually, but we'll leave it as-is for now.

This revision was automatically updated to reflect the committed changes.
epastor marked an inline comment as done.

It looks like this broke the Windows LLDB Buildbot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/14117

Already reverted in http://reviews.llvm.org/rG9fe769a961dc - apologies, I lost track and thought we'd moved to supporting C++17 code in the codebase. The version here included use of an initializer in an if-statement.

This will be landed again in another commit.