This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Remove datalayout strings from llc tests
ClosedPublic

Authored by tlively on Jul 12 2021, 1:46 PM.

Details

Summary

The data layout strings do not have any effect on llc tests and will become
misleadingly out of date as we continue to update the canonical data layout, so
remove them from the tests.

Diff Detail

Event Timeline

tlively created this revision.Jul 12 2021, 1:46 PM
tlively requested review of this revision.Jul 12 2021, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2021, 1:46 PM

Some of them seem to be opt tests..?

Some of them seem to be opt tests..?

Good catch. I had to keep the data layout string in some opt tests for them to pass, so I'll go through and make sure to keep the data layout string in all the opt tests.

tlively updated this revision to Diff 358068.Jul 12 2021, 2:28 PM
  • Restore data layout string in all opt tests

I'm ok with this change. I do hope we won't be continuing to update the canonical datalayout though.

Also I’m not sure if the data layout doesn’t really matter for llc tests, becuse llc also runs some IR passes. For example, WasmEHPrepare and LowerEmscriptenEHSjLj passes both run within llc and IIRC at least LowerEmscripten~ pass uses datalayout.

Also I’m not sure if the data layout doesn’t really matter for llc tests, becuse llc also runs some IR passes. For example, WasmEHPrepare and LowerEmscriptenEHSjLj passes both run within llc and IIRC at least LowerEmscripten~ pass uses datalayout.

Surely we always want to let llvm set the data layout based on the triple though don't we? Does opt not do this too? What data layout other than default one for the given triple could it rationally select?

aheejin accepted this revision.Jul 13 2021, 11:10 PM

I think that makes sense. Currently it looks if the data layout string is missing we have the default here: https://github.com/llvm/llvm-project/blob/dfd9808b6cea59ff075498ee7e6e57f2b5b3a798/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp#L125-L131

But because this is from WebAssemblyTargetMachine, this only works for llc and not opt. I think we need to keep opt strings but maybe not llc then.

This revision is now accepted and ready to land.Jul 13 2021, 11:10 PM
This revision was landed with ongoing or failed builds.Jul 14 2021, 11:17 AM
This revision was automatically updated to reflect the committed changes.