This is an archive of the discontinued LLVM Phabricator instance.

[X86][SSE2] Fix asm string for movq (Move Quadword) instruction
ClosedPublic

Authored by aymanmus on Apr 19 2017, 5:02 AM.

Diff Detail

Event Timeline

aymanmus created this revision.Apr 19 2017, 5:02 AM
igorb edited edge metadata.Apr 19 2017, 5:25 AM

Could you please add encoding tests for MOVPQIto64rr.

aymanmus added inline comments.Apr 19 2017, 7:10 AM
test/MC/X86/x86-64.s
1304

@igorb this is a test for MOVPQIto64rr.

  • Notice that I changed 6 instructions, 4 of them are marked as isCodeGenOnly=1, so basically these tests covers them all.
zvi edited edge metadata.Apr 19 2017, 9:52 AM

LGTM. Let Igor give the final ok.
Are the VEX/EVEX variants correct?

test/CodeGen/X86/vector-pcmp.ll
1

Please remove this line

test/MC/X86/x86-64.s
1303–1304

Is this test-case a duplicate of the above?

aymanmus updated this revision to Diff 95771.Apr 19 2017, 10:00 AM
aymanmus marked 2 inline comments as done.

@zvi, yes, the VEX/EVEX variants are correct.

RKSimon added inline comments.Apr 19 2017, 10:01 AM
test/CodeGen/X86/vector-pcmp.ll
1

If you can please commit the conversion to update_llc_test_checks.py (with trailing ; line removal) as an NFC pre-commit - this patch is big already!

craig.topper edited edge metadata.Apr 19 2017, 10:09 AM

This wasn't a mistake. We have aliases for movq for this instruction already. This was for compatibility with MacOS/Darwin's old assembler from the pre-clang days.

See this code

These are the correct encodings of the instructions so that we know how to
read correct assembly, even though we continue to emit the wrong ones for
// compatibility with Darwin's buggy assembler.
def : InstAlias<"movq\t{$src, $dst|$dst, $src}",

(MOV64toPQIrr VR128:$dst, GR64:$src), 0>;

def : InstAlias<"movq\t{$src, $dst|$dst, $src}",

(MOVPQIto64rr GR64:$dst, VR128:$src), 0>;

// Allow "vmovd" but print "vmovq" since we don't need compatibility for AVX.
def : InstAlias<"vmovd\t{$src, $dst|$dst, $src}",

(VMOV64toPQIrr VR128:$dst, GR64:$src), 0>;

def : InstAlias<"vmovd\t{$src, $dst|$dst, $src}",

(VMOVPQIto64rr GR64:$dst, VR128:$src), 0>;

@craig.topper is this still needed? Why would we insert a "bug" only to be compatible with MacOS?
What about the memory form instructions? InstAlias cannot solve this for them.

In the days before the MC layer of LLVM existed, clang would output assembly as text and feed it to the separate assembler tool. So it had to output what the MacOS assembler could parse.

I believe the assembler bug occurred around when 64-bit was introduced. There was already separate 64-bit movq to/from memory instruction before that. Then movd picked up a rex prefix that allowed it to access 64-bit registers and redudantly access 64-bit memory. I don't think there was ever a movq to memory issue because we never expect to use the rex encoded memory instruction since the older movq instruction is available.

We probably don't need to output that way anymore. But we should keep an alias around so we can process old assembly filed. So switch the instruction string to movq and change the alias to movd.

aymanmus marked an inline comment as done.Apr 23 2017, 12:58 AM
aymanmus updated this revision to Diff 96295.Apr 23 2017, 1:09 AM
craig.topper accepted this revision.Apr 25 2017, 8:39 AM

LGTM

lib/Target/X86/X86InstrSSE.td
4819

aliases is mispelled.

This revision is now accepted and ready to land.Apr 25 2017, 8:39 AM
This revision was automatically updated to reflect the committed changes.