This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add CMPCCXADD instructions.
ClosedPublic

Authored by FreddyYe on Oct 13 2022, 8:00 PM.

Diff Detail

Event Timeline

FreddyYe created this revision.Oct 13 2022, 8:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2022, 8:00 PM
FreddyYe requested review of this revision.Oct 13 2022, 8:00 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 13 2022, 8:00 PM
FreddyYe retitled this revision from Add CMPCCXADD instructions. to [X86] Add CMPCCXADD instructions..Oct 13 2022, 8:17 PM
RKSimon added inline comments.Oct 14 2022, 1:19 AM
clang/test/CodeGen/X86/cmpccxadd-builtins-error.c
3

Add 32-bit test coverage to ensure the intrinsics aren't visible?

RKSimon added inline comments.Oct 14 2022, 1:19 AM
llvm/CMakeLists.txt
887 ↗(On Diff #467668)

What is this for?

FreddyYe updated this revision to Diff 468395.Oct 17 2022, 7:25 PM
FreddyYe marked 2 inline comments as done.

Address comments.

craig.topper added inline comments.
clang/include/clang/Basic/BuiltinsX86_64.def
139

There was a blank line here. Put it back.

clang/lib/Basic/Targets/X86.cpp
781

What is CMPCCXADD_SUPPORTED for?

llvm/lib/Target/X86/X86.td
259

Why AVX2?

llvm/lib/Target/X86/X86InstrSSE.td
8117

This feels like it belongs somewhere other than X86InstrSSE.td since it's not vector related.

8130

Any possibility of doing this like how JCC_1, SETCCr, and CMOV32rr using an immediate for the lower 4 bits of the opcode?

8144–8145

Should there be aliases for consistency with Jcc, Setcc, and cmovcc. To support A, AE, GT, GE etc.?

craig.topper added inline comments.Oct 17 2022, 7:53 PM
llvm/lib/Target/X86/X86InstrSSE.td
8117

Missing Defs = [EFLAGS] I think

RKSimon added inline comments.Oct 18 2022, 2:30 AM
llvm/test/MC/X86/x86-64-cmpccxadd-att.s
1 ↗(On Diff #468395)

Drop the -att.s and add intel test coverage?

Matt added a subscriber: Matt.Oct 19 2022, 5:04 PM
FreddyYe updated this revision to Diff 469456.Oct 20 2022, 7:55 PM
FreddyYe marked 8 inline comments as done.

Address comments. THX for review!

llvm/lib/Target/X86/X86.td
259

Removed.

llvm/lib/Target/X86/X86InstrSSE.td
8117

Yes. Moved to llvm/lib/Target/X86/X86InstrCompiler.td

8130

Yes. Changed so.

8144–8145

Yes, changed so.

craig.topper added inline comments.Oct 20 2022, 9:05 PM
clang/lib/Basic/Targets/X86.cpp
970

This list is alphabetized or was supposed to be

1000

This list is alphabetized

llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
1071

This comment is out of date.

llvm/lib/Target/X86/X86ISelLowering.cpp
5631

Probably need MOVolatile too

llvm/lib/Target/X86/X86InstrCompiler.td
1026

X86InstrCompiler.td is for pseudos and isCodeGenOnly=1 instructions. Basically things only needed by CodeGen and not the assembler/disassembler.

llvm/utils/TableGen/X86RecognizableInstr.cpp
874

Extra space after MRMDestMem4VOp3CC

FreddyYe marked 4 inline comments as done.Oct 20 2022, 10:32 PM
FreddyYe added inline comments.
llvm/lib/Target/X86/X86InstrCompiler.td
1026

I was to make it near "cmpxchg". What about llvm/lib/Target/X86/X86InstrInfo.td?

FreddyYe updated this revision to Diff 469470.Oct 20 2022, 10:44 PM
FreddyYe marked 2 inline comments as done.

Address comments.

craig.topper added inline comments.Oct 20 2022, 11:17 PM
llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
1071

I'd prefer if you fixed the comment rather than deleting it.

llvm/lib/Target/X86/X86InstrInfo.td
3017

Can we put $dstsrc2 before $dstsrc1 in the ins list? That would remove the need for the change in getOperandBias I think. But maybe it complicates the encoder and disassembler?

llvm/lib/Target/X86/X86InstrSSE.td
8117

Looks like there's a blank line being deleted here?

pengfei added inline comments.Oct 21 2022, 1:39 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
5624–5634

This can be merged with above.

skan added inline comments.Oct 21 2022, 1:49 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
28101

Unused?

llvm/lib/Target/X86/X86InstrInfo.td
3014

Should we set isCodeGenOnly =1 here?

llvm/utils/TableGen/X86RecognizableInstr.h
109

Should we use 20 here?

skan added inline comments.Oct 21 2022, 2:01 AM
llvm/lib/Target/X86/X86InstrInfo.td
3018

+1 for craig. Usually if the input is tied to ouput, it should be the 1st input. These two instructions use a new Format MRMDestMem4VOp3CC rather than an existing one, I belive it won't increase the complexity.

FreddyYe updated this revision to Diff 469979.Oct 23 2022, 5:42 AM
FreddyYe marked 7 inline comments as done.

Address comments. THX for review!

pengfei added inline comments.Oct 23 2022, 6:32 AM
clang/test/Driver/x86-target-features.c
308

Use -target=xxx -march=xxx to match with others. xxx can be x86_64 I think.

309

ditto.

llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
1073

Unrelated change.

1128

Maybe move it to line 1133? MRMDestMem4VOp3CC is used for VEX encoding only.

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
816

Also this is not needed.

820

ditto.

llvm/lib/Target/X86/X86ISelLowering.cpp
34034

ditto.

llvm/lib/Target/X86/X86ISelLowering.h
872

Add comments for them?

873

This node seems not used.

llvm/lib/Target/X86/X86InstrInfo.td
348

Change to CMPCCXADD since it's shared.

FreddyYe updated this revision to Diff 470027.Oct 23 2022, 6:54 PM
FreddyYe marked 10 inline comments as done.

Address comments. THX for review!

skan added inline comments.Oct 23 2022, 7:40 PM
llvm/lib/Target/X86/X86InstrInfo.td
3025

set GR64:$dst, EFLAGS ...?

skan added inline comments.Oct 23 2022, 7:50 PM
clang/lib/Headers/cmpccxaddintrin.h
20–35

Could you use the same suffix for the condition code as ./llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h? e.g

NB->AE
Z->E
NZ->NE
NBE->A

and so on.

craig.topper added inline comments.Oct 23 2022, 8:18 PM
clang/lib/Headers/cmpccxaddintrin.h
20–35

Probably should have both versions as aliases.

llvm/lib/Target/X86/X86InstrInfo.td
3025

That doesn't work unless X86cmpccxadd is declare as having two results.

FreddyYe updated this revision to Diff 470051.Oct 23 2022, 10:38 PM
FreddyYe marked 5 inline comments as done.

Address comments. THX for review!

FreddyYe added inline comments.Oct 23 2022, 10:41 PM
clang/lib/Headers/cmpccxaddintrin.h
20–35

Yes, agree to add both,

llvm/lib/Target/X86/X86InstrInfo.td
3025

I did a test. It won't affect this intrinsic's lowering. Since it's not be useful and only for intrinsic lowering, I preferred to not add.

FreddyYe updated this revision to Diff 470079.Oct 24 2022, 1:17 AM

Support different default condition code for CMPCCXADD.

skan added inline comments.Oct 24 2022, 1:40 AM
llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp
32 ↗(On Diff #470079)

Capitalize 1st char

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
1455–1457

Minor suggestion

emitMemModRMByte(MI, ++CurOp, getX86RegNum(MI.getOperand(0)), TSFlags,
                 HasREX, StartByte, OS, Fixups, STI, false);
CurOp = SrcRegNum + 2; // skip VEX_V4 and CC

would be more clear b/c you use "skip VEX_V4 and CC" in the comments.

pengfei accepted this revision.Oct 24 2022, 1:45 AM

LGTM.

llvm/lib/Target/X86/X86InstrInfo.td
3014

Should also set mayLoad and mayStore?

This revision is now accepted and ready to land.Oct 24 2022, 1:45 AM
skan added inline comments.Oct 24 2022, 1:50 AM
llvm/lib/Target/X86/X86InstrInfo.td
3014

Yeah, I think so.

3017

Could you use "GR32:$dstsrc1, i32mem:$dstsrc2" instead?

FreddyYe updated this revision to Diff 470110.Oct 24 2022, 4:17 AM
FreddyYe marked 5 inline comments as done.

Address comments. THX for review!

FreddyYe added inline comments.Oct 24 2022, 4:21 AM
llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
1455–1457

We cannot do ++ before emitMemModRMByte, so there is a implicit +1 for that, like other cases.

llvm/lib/Target/X86/X86InstrInfo.td
3017

OK, I'm ok to either. Then line below should do an reversion.

FreddyYe updated this revision to Diff 470363.Oct 24 2022, 8:46 PM

Corrected comments.

skan accepted this revision.Oct 24 2022, 10:38 PM

LGTM

This revision was landed with ongoing or failed builds.Oct 24 2022, 11:57 PM
This revision was automatically updated to reflect the committed changes.