This is an archive of the discontinued LLVM Phabricator instance.

[AVX-512] Disassembler support for rounding control and SAE attribute.
AbandonedPublic

Authored by maksfb on Oct 17 2016, 12:26 PM.

Details

Summary

Add missing disassembler support for EVEX-encoded instructions with rounding control and/or SAE attribute.

Diff Detail

Event Timeline

maksfb updated this revision to Diff 74809.Oct 17 2016, 12:26 PM
maksfb retitled this revision from to [AVX-512] Disassembler support for rounding control and SAE attribute..
maksfb updated this object.
maksfb added reviewers: craig.topper, delena, nadav.
craig.topper edited edge metadata.Oct 17 2016, 8:45 PM

I think this looks ok, but I'd like Elena to look at it too.

@delena : could you please take a look?

delena added inline comments.Oct 25 2016, 12:37 PM
lib/Target/X86/X86InstrAVX512.td
1886

I don't see LIG specification in the "sae" form of VCMP:

EVEX.NDS.512.66.0F.W1 C2 /r ib FV V/V AVX512F Compare packed double-precision floating-point values
in zmm3/m512/m64bcst and zmm2 using bits 4:0 of
imm8 as a comparison predicate with writemask k2
and leave the result in mask register k1.
VCMPPD k1 {k2}, zmm2,
zmm3/m512/m64bcst{sae}, imm8

5817

Why do you need to remove EVEX_V512 from the second pattern?

maksfb marked 2 inline comments as done.Oct 25 2016, 3:16 PM
maksfb added inline comments.
lib/Target/X86/X86InstrAVX512.td
1886

The spec for SAE says it "applies to scalar and 512-bit vector lengths". I don't know how CPU actually treats L'L when {sae} is set, except that LLVM assembler generates {0,0}, not {1,0} that corresponds to 512-bit length.

As a result if you feed assembler output to disassembler it causes disassembler to fail.

I can speculate that LIG is implied in this case, but folks at Intel would know better. Could you check?

5817

Because RC implies 512-bit length ("Vector Length Orthogonality"), and putting EVEX_V512 enforces disassembler to accept L'L={1,0} only, while LLVM assembler generates {0,0} IIRC.

delena added inline comments.Oct 26 2016, 2:27 AM
lib/Target/X86/X86InstrAVX512.td
1886

According to table 4-7 in the spec:
FP Instructions w/o rounding semantic:
EVEX_B - SAE control
EVEX.L’L 00, 01, 10 - for 128/256/512.

5817

I read the spec again. "When EVEX is used to encode scalar instructions, L’L is generally ignored"
It means that you can put VEX_LIG to all scalars and remove EVEX_V512 from all scalar forms, right?

utils/TableGen/X86RecognizableInstr.cpp
996

Why IMM8 ?

maksfb marked 2 inline comments as done.Oct 26 2016, 9:14 AM
maksfb added inline comments.
lib/Target/X86/X86InstrAVX512.td
1886

Does it make the spec wrong? Is there any non-512-bit vector instruction with SAE?

5817

Probably.

utils/TableGen/X86RecognizableInstr.cpp
996

Made more sense than IMM32 since it saves some bytes.

delena added inline comments.Oct 26 2016, 11:57 AM
lib/Target/X86/X86InstrAVX512.td
1886
  1. You actually changed instruction encoding. We have a lot of encoding tests. Do you want to say that our tests do not cover these cases?
  2. Could you, please, check what encoding is generated by GCC?
  3. In order to be sure that the HW ignores L'L you can check with XED or SDE. XED is a part of SDE package which is free on Intel site.
utils/TableGen/X86RecognizableInstr.cpp
996

Where?

I think the focus should be on fixing the state of LLVM's disassembler for AVX-512. While there could be issues with the encoder too, and it's important to find out if indeed the encoder has to be fixed, that has to be handled separately. This diff fixes multiple failures of LLVM disassembler while handling binaries with AVX-512, but it does not fix them all.

Since you have encoding tests - you can pass the output of assembler to disassembler. It's one line in .test file(s).

lib/Target/X86/X86InstrAVX512.td
1886
  1. How? If you think I've changed the encoding could you provide an example? Do you have a test case that checks SAE encoding? What does it expect L'L to be? I'm based on rev from Oct 11th and all tests pass.
  2. as from binutils 2.26.1 generates {0,0}.
  3. I agree, it's important to make sure LLVM codegen for Intel's extension is not broken.
delena edited edge metadata.Oct 27 2016, 1:59 AM

Can you send me old and new encoding of vmpps with {sae}?
I'll try to check.

I also sent a question to our architects.

Why do you need VEX_LIG for disassembler?
EVEX_B in reg-reg form says that you have SAE.
EVEX_RC says that you have RC.

I agree with Elena, we should use the other flags to ignore the vector size in the disassembler table generation. This is what we already do in the encoder. Let's keep the intention of the L and L2 bits in TSFlags as they are.

maksfb abandoned this revision.Jan 18 2022, 7:32 PM