[PATCH] D135933: [X86] Add CMPCCXADD instructions.
skan accepted this revision. skan added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135933/new/ https://reviews.llvm.org/D135933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
FreddyYe added inline comments.
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:1453-1455
+emitMemModRMByte(MI, CurOp + 1, getX86RegNum(MI.getOperand(0)), TSFlags,
+ HasREX, StartByte, OS, Fixups, STI, false);
+CurOp = SrcRegNum + 3; // skip VEX_V4 and CC
skan wrote:
> 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.
We cannot do `++` before emitMemModRMByte, so there is a implicit +1 for that,
like other cases.
Comment at: llvm/lib/Target/X86/X86InstrInfo.td:3027
+def CMPCCXADDmr32 : I<0xe0, MRMDestMem4VOp3CC, (outs GR32:$dst),
+ (ins GR32:$dstsrc2, i32mem:$dstsrc1, GR32:$src3, ccode:$cond),
+ "cmp${cond}xadd\t{$src3, $dst, $dstsrc1|$dstsrc1, $dst, $src3}",
skan wrote:
> Could you use "GR32:$dstsrc1, i32mem:$dstsrc2" instead?
OK, I'm ok to either. Then line below should do an reversion.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
skan added inline comments.
Comment at: llvm/lib/Target/X86/X86InstrInfo.td:3027
+def CMPCCXADDmr32 : I<0xe0, MRMDestMem4VOp3CC, (outs GR32:$dst),
+ (ins GR32:$dstsrc2, i32mem:$dstsrc1, GR32:$src3, ccode:$cond),
+ "cmp${cond}xadd\t{$src3, $dst, $dstsrc1|$dstsrc1, $dst, $src3}",
Could you use "GR32:$dstsrc1, i32mem:$dstsrc2" instead?
Comment at: llvm/lib/Target/X86/X86InstrInfo.td:3024
+//
+let Predicates = [HasCMPCCXADD, In64BitMode], Defs = [EFLAGS],
+Constraints = "$dstsrc2 = $dst" in {
pengfei wrote:
> skan wrote:
> > Should we set isCodeGenOnly =1 here?
> Should also set `mayLoad` and `mayStore`?
Yeah, I think so.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
pengfei accepted this revision.
pengfei added a comment.
This revision is now accepted and ready to land.
LGTM.
Comment at: llvm/lib/Target/X86/X86InstrInfo.td:3024
+//
+let Predicates = [HasCMPCCXADD, In64BitMode], Defs = [EFLAGS],
+Constraints = "$dstsrc2 = $dst" in {
skan wrote:
> Should we set isCodeGenOnly =1 here?
Should also set `mayLoad` and `mayStore`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
skan added inline comments.
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp:32
int64_t Imm = MI->getOperand(Op).getImm();
+ bool flavor = MI->getOpcode() == X86::CMPCCXADDmr32 || X86::CMPCCXADDmr64;
switch (Imm) {
Capitalize 1st char
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:1453-1455
+emitMemModRMByte(MI, CurOp + 1, getX86RegNum(MI.getOperand(0)), TSFlags,
+ HasREX, StartByte, OS, Fixups, STI, false);
+CurOp = SrcRegNum + 3; // skip VEX_V4 and CC
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
FreddyYe added inline comments.
Comment at: clang/lib/Headers/cmpccxaddintrin.h:19-34
+ _CMPCCX_O, /* Overflow. */
+ _CMPCCX_NO, /* No overflow. */
+ _CMPCCX_B, /* Below. */
+ _CMPCCX_NB, /* Not below. */
+ _CMPCCX_Z, /* Zero. */
+ _CMPCCX_NZ, /* Not zero. */
+ _CMPCCX_BE, /* Below or equal. */
craig.topper wrote:
> skan wrote:
> > 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.
> Probably should have both versions as aliases.
Yes, agree to add both,
Comment at: llvm/lib/Target/X86/X86InstrInfo.td:3035
+ "cmp${cond}xadd\t{$src3, $dst, $dstsrc1|$dstsrc1, $dst, $src3}",
+ [(set GR64:$dst, (X86cmpccxadd addr:$dstsrc1, GR64:$dstsrc2,
GR64:$src3, timm:$cond))]>,
+ VEX_4V, VEX_W, T8PD, Sched<[WriteXCHG]>;
craig.topper wrote:
> skan wrote:
> > `set GR64:$dst, EFLAGS ...`?
> That doesn't work unless X86cmpccxadd is declare as having two results.
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
craig.topper added inline comments.
Comment at: clang/lib/Headers/cmpccxaddintrin.h:19-34
+ _CMPCCX_O, /* Overflow. */
+ _CMPCCX_NO, /* No overflow. */
+ _CMPCCX_B, /* Below. */
+ _CMPCCX_NB, /* Not below. */
+ _CMPCCX_Z, /* Zero. */
+ _CMPCCX_NZ, /* Not zero. */
+ _CMPCCX_BE, /* Below or equal. */
skan wrote:
> 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.
Probably should have both versions as aliases.
Comment at: llvm/lib/Target/X86/X86InstrInfo.td:3035
+ "cmp${cond}xadd\t{$src3, $dst, $dstsrc1|$dstsrc1, $dst, $src3}",
+ [(set GR64:$dst, (X86cmpccxadd addr:$dstsrc1, GR64:$dstsrc2,
GR64:$src3, timm:$cond))]>,
+ VEX_4V, VEX_W, T8PD, Sched<[WriteXCHG]>;
skan wrote:
> `set GR64:$dst, EFLAGS ...`?
That doesn't work unless X86cmpccxadd is declare as having two results.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
skan added inline comments. Comment at: clang/lib/Headers/cmpccxaddintrin.h:19-34 + _CMPCCX_O, /* Overflow. */ + _CMPCCX_NO, /* No overflow. */ + _CMPCCX_B, /* Below. */ + _CMPCCX_NB, /* Not below. */ + _CMPCCX_Z, /* Zero. */ + _CMPCCX_NZ, /* Not zero. */ + _CMPCCX_BE, /* Below or equal. */ 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135933/new/ https://reviews.llvm.org/D135933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
skan added inline comments.
Comment at: llvm/lib/Target/X86/X86InstrInfo.td:3035
+ "cmp${cond}xadd\t{$src3, $dst, $dstsrc1|$dstsrc1, $dst, $src3}",
+ [(set GR64:$dst, (X86cmpccxadd addr:$dstsrc1, GR64:$dstsrc2,
GR64:$src3, timm:$cond))]>,
+ VEX_4V, VEX_W, T8PD, Sched<[WriteXCHG]>;
`set GR64:$dst, EFLAGS ...`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
pengfei added inline comments. Comment at: clang/test/Driver/x86-target-features.c:313 +// RUN: %clang -target i686-unknown-linux-gnu -mcmpccxadd %s -### -o %t.o 2>&1 | FileCheck -check-prefix=CMPCCXADD %s +// RUN: %clang -target i686-unknown-linux-gnu -mno-cmpccxadd %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-CMPCCXADD %s Use -target=xxx -march=xxx to match with others. xxx can be x86_64 I think. Comment at: clang/test/Driver/x86-target-features.c:314 +// RUN: %clang -target i686-unknown-linux-gnu -mcmpccxadd %s -### -o %t.o 2>&1 | FileCheck -check-prefix=CMPCCXADD %s +// RUN: %clang -target i686-unknown-linux-gnu -mno-cmpccxadd %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-CMPCCXADD %s +// CMPCCXADD: "-target-feature" "+cmpccxadd" ditto. Comment at: llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h:1073 // operand. - if (NumOps == 8 && + if ((NumOps == 8) && Desc.getOperandConstraint(6, MCOI::TIED_TO) == 0) Unrelated change. Comment at: llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h:1127 case X86II::MRMSrcMem4VOp3: +case X86II::MRMDestMem4VOp3CC: // Skip registers encoded in reg. Maybe move it to line 1133? `MRMDestMem4VOp3CC` is used for VEX encoding only. Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:816 +VEX_R = ~(RegEnc >> 3) & 1; +EVEX_R2 = ~(RegEnc >> 4) & 1; + Also this is not needed. Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:820 +VEX_4V = ~VRegEnc & 0xf; +EVEX_V2 = ~(VRegEnc >> 4) & 1; +break; ditto. Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:34174 + NODE_NAME_CASE(CMPCCXADD32) + NODE_NAME_CASE(CMPCCXADD64) NODE_NAME_CASE(TESTUI) ditto. Comment at: llvm/lib/Target/X86/X86ISelLowering.h:872 +CMPCCXADD32, +CMPCCXADD64, Add comments for them? Comment at: llvm/lib/Target/X86/X86ISelLowering.h:873 +CMPCCXADD32, +CMPCCXADD64, + This node seems not used. Comment at: llvm/lib/Target/X86/X86InstrInfo.td:348 +def X86cmpccxadd : SDNode<"X86ISD::CMPCCXADD32", SDTX86Cmpccxadd, + [SDNPHasChain, SDNPMayLoad, SDNPMayStore, Change to `CMPCCXADD` since it's shared. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135933/new/ https://reviews.llvm.org/D135933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
skan added inline comments.
Comment at: llvm/lib/Target/X86/X86InstrInfo.td:3028
+ (ins i32mem:$dstsrc1, GR32:$dstsrc2, GR32:$src3, ccode:$cond),
+ "cmp${cond}xadd\t{$src3, $dst, $dstsrc1|$dstsrc1, $dst, $src3}",
+ [(set GR32:$dst, (X86cmpccxadd addr:$dstsrc1, GR32:$dstsrc2,
GR32:$src3, timm:$cond))]>,
+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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
skan added inline comments.
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:28251
+ SDLoc DL(Op);
+ SDVTList VTs = DAG.getVTList(MVT::i32, MVT::Other);
+ SDValue Chain = Op.getOperand(0);
Unused?
Comment at: llvm/lib/Target/X86/X86InstrInfo.td:3024
+//
+let Predicates = [HasCMPCCXADD, In64BitMode], Defs = [EFLAGS],
+Constraints = "$dstsrc2 = $dst" in {
Should we set isCodeGenOnly =1 here?
Comment at: llvm/utils/TableGen/X86RecognizableInstr.h:109
PrefixByte= 10,
+MRMDestMem4VOp3CC = 18,
MRMr0 = 21,
Should we use 20 here?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
pengfei added inline comments.
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:5643-5653
+case Intrinsic::x86_cmpccxadd32:
+case Intrinsic::x86_cmpccxadd64: {
+ Info.opc = ISD::INTRINSIC_W_CHAIN;
+ Info.ptrVal = I.getArgOperand(0);
+ unsigned Size = I.getType()->getScalarSizeInBits();
+ Info.memVT = EVT::getIntegerVT(I.getType()->getContext(), Size);
+ Info.align = Align(Size);
This can be merged with above.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
craig.topper added inline comments.
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h:1071
return 1;
// Check for AVX-512 scatter which has a TIED_TO in the second to last
// operand.
craig.topper wrote:
> This comment is out of date.
I'd prefer if you fixed the comment rather than deleting it.
Comment at: llvm/lib/Target/X86/X86InstrInfo.td:3027
+def CMPCCXADDmr32 : I<0xe0, MRMDestMem4VOp3CC, (outs GR32:$dst),
+ (ins i32mem:$dstsrc1, GR32:$dstsrc2, GR32:$src3, ccode:$cond),
+ "cmp${cond}xadd\t{$src3, $dst, $dstsrc1|$dstsrc1, $dst, $src3}",
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?
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:8125
}
-
Looks like there's a blank line being deleted here?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
FreddyYe marked 4 inline comments as done.
FreddyYe added inline comments.
Comment at: llvm/lib/Target/X86/X86InstrCompiler.td:1026
+let Predicates = [HasCMPCCXADD, In64BitMode], Defs = [EFLAGS],
+Constraints = "$dstsrc2 = $dst" in {
craig.topper wrote:
> X86InstrCompiler.td is for pseudos and isCodeGenOnly=1 instructions.
> Basically things only needed by CodeGen and not the assembler/disassembler.
I was to make it near "cmpxchg". What about llvm/lib/Target/X86/X86InstrInfo.td?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
craig.topper added inline comments.
Comment at: clang/lib/Basic/Targets/X86.cpp:970
.Case("xsaveopt", true)
+ .Case("cmpccxadd", true)
.Default(false);
This list is alphabetized or was supposed to be
Comment at: clang/lib/Basic/Targets/X86.cpp:1000
.Case("avx512vp2intersect", HasAVX512VP2INTERSECT)
+ .Case("cmpccxadd", HasCMPCCXADD)
.Case("bmi", HasBMI)
This list is alphabetized
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h:1071
return 1;
// Check for AVX-512 scatter which has a TIED_TO in the second to last
// operand.
This comment is out of date.
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:5631
+ Info.align = Align(Size);
+ Info.flags |= MachineMemOperand::MOLoad | MachineMemOperand::MOStore;
+ return true;
Probably need MOVolatile too
Comment at: llvm/lib/Target/X86/X86InstrCompiler.td:1026
+let Predicates = [HasCMPCCXADD, In64BitMode], Defs = [EFLAGS],
+Constraints = "$dstsrc2 = $dst" in {
X86InstrCompiler.td is for pseudos and isCodeGenOnly=1 instructions. Basically
things only needed by CodeGen and not the assembler/disassembler.
Comment at: llvm/utils/TableGen/X86RecognizableInstr.cpp:874
+ Form == X86Local::MRMXmCC || Form == X86Local::AddCCFrm ||
+ Form == X86Local::MRMDestMem4VOp3CC ) {
uint8_t Count = Form == X86Local::AddRegFrm ? 8 : 16;
Extra space after MRMDestMem4VOp3CC
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
FreddyYe added inline comments.
Comment at: llvm/lib/Target/X86/X86.td:259
+"Support CMPCCXADD instructions",
+[FeatureAVX2]>;
def FeatureINVPCID : SubtargetFeature<"invpcid", "HasINVPCID", "true",
craig.topper wrote:
> Why AVX2?
Removed.
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:8118
+let Predicates = [HasCMPCCXADD, In64BitMode], Constraints = "$dstsrc2 = $dst"
in
+multiclass CMPCCXADD_BASE Opc, string OpcodeStr> {
craig.topper wrote:
> craig.topper wrote:
> > This feels like it belongs somewhere other than X86InstrSSE.td since it's
> > not vector related.
> Missing `Defs = [EFLAGS]` I think
Yes. Moved to llvm/lib/Target/X86/X86InstrCompiler.td
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:8131
+
+defm CMPBEXADD : CMPCCXADD_BASE<0xe6, "cmpbexadd">;
+defm CMPBXADD : CMPCCXADD_BASE<0xe2, "cmpbxadd">;
craig.topper wrote:
> Any possibility of doing this like how JCC_1, SETCCr, and CMOV32rr using an
> immediate for the lower 4 bits of the opcode?
Yes. Changed so.
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:8145-8146
+defm CMPPXADD : CMPCCXADD_BASE<0xea, "cmppxadd">;
+defm CMPSXADD : CMPCCXADD_BASE<0xe8, "cmpsxadd">;
+defm CMPZXADD : CMPCCXADD_BASE<0xe4, "cmpzxadd">;
+
craig.topper wrote:
> Should there be aliases for consistency with Jcc, Setcc, and cmovcc. To
> support A, AE, GT, GE etc.?
Yes, changed so.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
RKSimon added inline comments. Comment at: llvm/test/MC/X86/x86-64-cmpccxadd-att.s:1 +// RUN: llvm-mc -triple x86_64 --show-encoding %s | FileCheck %s + Drop the -att.s and add intel test coverage? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135933/new/ https://reviews.llvm.org/D135933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
craig.topper added inline comments.
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:8118
+let Predicates = [HasCMPCCXADD, In64BitMode], Constraints = "$dstsrc2 = $dst"
in
+multiclass CMPCCXADD_BASE Opc, string OpcodeStr> {
craig.topper wrote:
> This feels like it belongs somewhere other than X86InstrSSE.td since it's not
> vector related.
Missing `Defs = [EFLAGS]` I think
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
craig.topper added inline comments. Comment at: clang/include/clang/Basic/BuiltinsX86_64.def:138 +TARGET_BUILTIN(__builtin_ia32_cmpccxadd64, "SLLiv*SLLiSLLiIi", "n", "cmpccxadd") #undef BUILTIN #undef TARGET_BUILTIN There was a blank line here. Put it back. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135933/new/ https://reviews.llvm.org/D135933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
craig.topper added inline comments.
Comment at: clang/lib/Basic/Targets/X86.cpp:781
+Builder.defineMacro("__CMPCCXADD__");
+ Builder.defineMacro("__CMPCCXADD_SUPPORTED__");
if (HasAVXVNNI)
What is __CMPCCXADD_SUPPORTED__ for?
Comment at: llvm/lib/Target/X86/X86.td:259
+"Support CMPCCXADD instructions",
+[FeatureAVX2]>;
def FeatureINVPCID : SubtargetFeature<"invpcid", "HasINVPCID", "true",
Why AVX2?
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:8118
+let Predicates = [HasCMPCCXADD, In64BitMode], Constraints = "$dstsrc2 = $dst"
in
+multiclass CMPCCXADD_BASE Opc, string OpcodeStr> {
This feels like it belongs somewhere other than X86InstrSSE.td since it's not
vector related.
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:8131
+
+defm CMPBEXADD : CMPCCXADD_BASE<0xe6, "cmpbexadd">;
+defm CMPBXADD : CMPCCXADD_BASE<0xe2, "cmpbxadd">;
Any possibility of doing this like how JCC_1, SETCCr, and CMOV32rr using an
immediate for the lower 4 bits of the opcode?
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:8145-8146
+defm CMPPXADD : CMPCCXADD_BASE<0xea, "cmppxadd">;
+defm CMPSXADD : CMPCCXADD_BASE<0xe8, "cmpsxadd">;
+defm CMPZXADD : CMPCCXADD_BASE<0xe4, "cmpzxadd">;
+
Should there be aliases for consistency with Jcc, Setcc, and cmovcc. To support
A, AE, GT, GE etc.?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135933: [X86] Add CMPCCXADD instructions.
RKSimon added inline comments.
Comment at: clang/test/CodeGen/X86/cmpccxadd-builtins-error.c:2
+// RUN: %clang_cc1 %s -ffreestanding -triple=x86_64-unknown-unknown \
+// RUN: -target-feature +cmpccxadd -emit-llvm -fsyntax-only -verify
+
Add 32-bit test coverage to ensure the intrinsics aren't visible?
Comment at: llvm/CMakeLists.txt:887
+ list(APPEND features_enabled_by_default
+ ${LLVM_INTELFEATURE_PREFIX}_ISA_CMPCCXADD)
What is this for?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135933/new/
https://reviews.llvm.org/D135933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
