[PATCH] D116072: [X86] GLC: Break false dependency for dest register for several instructions.

Fri Mar 18 02:58:23 PDT 2022

gpei marked 2 inline comments as done.
gpei added inline comments.

================
Comment at: llvm/lib/Target/X86/X86.td:447
                                      "LZCNT/TZCNT have a false dependency on dest register">;

 // On recent X86 (port bound) processors, its preferable to combine to a single shuffle
----------------
RKSimon wrote:
> (style) Keep all the FalseDeps tuning flags together?
Done, thanks for pointing this!

================
Comment at: llvm/lib/Target/X86/X86.td:583
+                               "HasCMULFalseDeps", "true",
+                               "CMUL has a false dependency on dest register">;
+
----------------
craig.topper wrote:
> Probably shouldn't overly shorten instruction names in documentation that goes into user facing help text.
You are right, have fixed it.

================
Comment at: llvm/lib/Target/X86/X86.td:999
   list<SubtargetFeature> ADLFeatures =
     !listconcat(TRMFeatures, ADLAdditionalFeatures);

----------------
RKSimon wrote:
> Out of interest - why is Alderlake defined with the atom core intel defs? I realise its big-little, but I'd still count it as a mainstream core and not an embedded budget core.
Sorry, I have no ideas, maybe it's caused by HW limitations.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:5785
+      return;
+    // These instructions are all floating point domain, so vpxord is the best
+    // choice.
----------------
craig.topper wrote:
> gpei wrote:
> > craig.topper wrote:
> > > This doesn't make sense. VPXORD is an integer instruction.
> > Yes, I copied the comments above. There is no VPXORPSZ128rr, is it acceptable I only delete my comments?
> That's because VP means integer. There's a VXORPSZ128rr, but it requires AVX512DQ. So using VPXORD is probably best.
OK, I know, thanks!

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116072/new/

https://reviews.llvm.org/D116072