This is an archive of the discontinued LLVM Phabricator instance.

[X86] Merge the different CMOV instructions for each condition code into single instructions that store the condition code as an immediate.
ClosedPublic

Authored by craig.topper on Mar 31 2019, 12:17 AM.

Details

Summary

Reorder the condition code enum to match their encodings. Move it to MC layer so it can be used by the scheduler models.

This avoids needing an isel pattern for each condition code. And it removes
translation switches for converting between CMOV instructions and condition
codes.

Now the printer, encoder and disassembler take care of converting the immediate.
We use InstAliases to handle the assembly matching. But we print using the
asm string in the instruction definition. The instruction itself is marked
IsCodeGenOnly=1 to hide it from the assembly parser.

This does complicate the scheduler models a little since we can't assign the
A and BE instructions to a separate class now.

I plan to make similar changes for SETcc and Jcc.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 31 2019, 12:17 AM

Observation: this affects llvm-exegesis.
At least, this diff is needed:

diff --git a/tools/llvm-exegesis/lib/X86/Target.cpp b/tools/llvm-exegesis/lib/X86/Target.cpp
index 369ed2f97d7..3acde820c37 100644
--- a/tools/llvm-exegesis/lib/X86/Target.cpp
+++ b/tools/llvm-exegesis/lib/X86/Target.cpp
@@ -32,6 +32,7 @@ static Error isInvalidMemoryInstr(const Instruction &Instr) {
   case X86II::MRMSrcReg:
   case X86II::MRMSrcReg4VOp3:
   case X86II::MRMSrcRegOp4:
+  case X86II::MRMSrcRegCC:
   case X86II::MRMXr:
   case X86II::MRM0r:
   case X86II::MRM1r:
@@ -118,6 +119,7 @@ static Error isInvalidMemoryInstr(const Instruction &Instr) {
   case X86II::MRMSrcMem:
   case X86II::MRMSrcMem4VOp3:
   case X86II::MRMSrcMemOp4:
+  case X86II::MRMSrcMemCC:
   case X86II::MRMXm:
   case X86II::MRM0m:
   case X86II::MRM1m:

But even then, unlike other instructions with immediates for whom it 'correctly' sets imm to 1,
it just crashes here:

$ ./bin/llvm-exegesis -mode=latency -opcode-name=ADD32ri
Check generated assembly with: /usr/bin/objdump -d /tmp/snippet-ee6c66.o
---
mode:            latency
key:             
  instructions:    
    - 'ADD32ri R12D R12D i_0x1'
  config:          ''
  register_initial_values: 
    - 'R12D=0x0'
cpu_name:        bdver2
llvm_triple:     x86_64-unknown-linux-gnu
num_repetitions: 10000
measurements:    
  - { key: latency, value: 1.5217, per_snippet_value: 1.5217 }
error:           ''
info:            Repeating a single implicitly serial instruction
assembled_snippet: 415441BC000000004181C4010000004181C4010000004181C4010000004181C4010000004181C4010000004181C4010000004181C4010000004181C4010000004181C4010000004181C4010000004181C4010000004181C4010000004181C4010000004181C4010000004181C4010000004181C401000000415CC3
...
$ ./bin/llvm-exegesis -mode=latency -opcode-name=CMOV32rr 
Operand is not set
UNREACHABLE executed at /build/llvm/tools/llvm-exegesis/lib/Assembler.cpp:114!
Aborted

(i'll leave out of the comment the fact that llvm-exegesis currently does not try to
check all the condcodes, i was actually going to try to experiment with that here..)

-Add the new formats to llvm-exegesis enum.
-Give the new condition code operand an OperandType other than OPERAND_UNKNOWN. Now it its own X86 specific value. Maybe llvm-exegesis can make use of this to select appropriate values.

craig.topper marked an inline comment as done.Mar 31 2019, 11:12 PM
craig.topper added inline comments.
lib/Target/X86/X86SchedPredicates.td
68 ↗(On Diff #193031)

I was trying to use CheckNumOperands to distinquish Memory from Register. But I don't think it works correctly with the implicit operands that MachineInstrs have. The MCInst version and the MachineInstr version of CMOV have different number operands since MachineInstr models the EFLAGS implicit use but MCInst doesn't.

Separate the schedler predicates for register and memory form. I don't think I can rely on num operands to distinquish them.

-Give the new condition code operand an OperandType other than OPERAND_UNKNOWN. Now it its own X86 specific value. Maybe llvm-exegesis can make use of this to select appropriate values.

Yep, that helped, thanks!

I have no objections to this but given the problems it raises to llvm-exegesis, would it be possible to add the setcc/jcc patches as child revisions so @courbet and @lebedev.ri can plan out the necessary changes?

I have no objections to this but given the problems it raises to llvm-exegesis, would it be possible to add the setcc/jcc patches as child revisions so @courbet and @lebedev.ri can plan out the necessary changes?

I'm almost done with the setcc patch. I just need to correct the scheduler models for it. I can hopefully post it this evening. Then I'll start on jcc.

Rebase after also providing a separate OperandType for AVX512 rounding control

@lebedev.ri and @courbet Are you happy with this and D60138?

@lebedev.ri and @courbet Are you happy with this and D60138?

Yes, a specific OPERAND_TYPE lets us handle this correctly, even there are still details that are currently being ironed out by @gchatelet and @lebedev.ri.

@lebedev.ri and @courbet Are you happy with this and D60138?

Yes, a specific OPERAND_TYPE lets us handle this correctly, even there are still details that are currently being ironed out by @gchatelet and @lebedev.ri.

SGTM. I'm not sure these patches should wait for exegesis patches, because exegesis patches are certainly waiting for these patches :)
Things aren't great with modelling immediates already, so one more opcode won't really make things that much worse.

RKSimon accepted this revision.Apr 4 2019, 12:34 AM

OK, seems everyone's happy and ready to make llvm-exegesis changes once this is in - LGTM

This revision is now accepted and ready to land.Apr 4 2019, 12:34 AM
This revision was automatically updated to reflect the committed changes.