[PATCH] D44386: [x86] Introduce the pconfig/enclv instructions

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Support/Host.cpp:1268
+  // leaves using cpuid, since that information is ignored while
+  // detecting features using the "-mnative" flag.
+  // For more info, see X86 ISA docs.
----------------
Did you mean -march=native?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:21163
+
+  SDValue a = DAG.getCopyFromReg(SDValue(Res, 0), dl, X86::EAX, MVT::i32, SDValue(Res, 1));
+  SDValue b = DAG.getCopyFromReg(a.getValue(1), dl, ArgReg1, ArgVT, a.getValue(2));
----------------
Variable names should be uppercase.


================
Comment at: lib/Target/X86/X86ISelLowering.h:567
 
+      // Platform Configuration
+      PCONFIG,
----------------
These aren't used by your patch since you selected the machine node directly.


================
Comment at: lib/Target/X86/X86IntrinsicsInfo.h:337
   X86_INTRINSIC_DATA(avx512_scattersiv8_si, SCATTER, X86::VPSCATTERDDZ256mr, 0),
+  X86_INTRINSIC_DATA(encls_32, ENCLS, X86ISD::ENCLS, 0),
+  X86_INTRINSIC_DATA(encls_64, ENCLS, X86ISD::ENCLS, 0),
----------------
You're not really using the X86ISD opcodes listed in the table here. You're probably better off not putting these in the table and putting the intrinsics in the earlier switch that's used when the table doesn't have an entry. The one that handles Intrinsic::x86_lwpins64


================
Comment at: utils/TableGen/X86RecognizableInstr.cpp:681
   case X86Local::MRM_C0: case X86Local::MRM_C1: case X86Local::MRM_C2:
-  case X86Local::MRM_C3: case X86Local::MRM_C4: case X86Local::MRM_C8:
-  case X86Local::MRM_C9: case X86Local::MRM_CA: case X86Local::MRM_CB:
-  case X86Local::MRM_CF: case X86Local::MRM_D0: case X86Local::MRM_D1:
-  case X86Local::MRM_D4: case X86Local::MRM_D5: case X86Local::MRM_D6:
-  case X86Local::MRM_D7: case X86Local::MRM_D8: case X86Local::MRM_D9:
-  case X86Local::MRM_DA: case X86Local::MRM_DB: case X86Local::MRM_DC:
-  case X86Local::MRM_DD: case X86Local::MRM_DE: case X86Local::MRM_DF:
-  case X86Local::MRM_E0: case X86Local::MRM_E1: case X86Local::MRM_E2:
-  case X86Local::MRM_E3: case X86Local::MRM_E4: case X86Local::MRM_E5:
-  case X86Local::MRM_E8: case X86Local::MRM_E9: case X86Local::MRM_EA:
-  case X86Local::MRM_EB: case X86Local::MRM_EC: case X86Local::MRM_ED:
-  case X86Local::MRM_EE: case X86Local::MRM_EF: case X86Local::MRM_F0:
-  case X86Local::MRM_F1: case X86Local::MRM_F2: case X86Local::MRM_F3:
-  case X86Local::MRM_F4: case X86Local::MRM_F5: case X86Local::MRM_F6:
-  case X86Local::MRM_F7: case X86Local::MRM_F9: case X86Local::MRM_FA:
-  case X86Local::MRM_FB: case X86Local::MRM_FC: case X86Local::MRM_FD:
-  case X86Local::MRM_FE: case X86Local::MRM_FF:
+  case X86Local::MRM_C3: case X86Local::MRM_C4: case X86Local::MRM_C5:
+  case X86Local::MRM_C8: case X86Local::MRM_C9: case X86Local::MRM_CA:
----------------
We should just add all of C8-FF into this switch. They are all defined not sure why they aren't already here. I'll go ahead and do that ahead of this patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D44386