[PATCH] D48225: [llvm-mca][X86] Teach how to identify register writes that implicitly clear the upper portion of a super-register.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

In https://reviews.llvm.org/D48225#1133813, @RKSimon wrote:

> Should there be some AVX512VL tests? We don't have any scheduler that can test XOP instructions AFAICT (unless we want to cheat and use SandyBridge in its role as the generic model).


Thanks Simon,

I will add a test for AVX512VL. If you have a good idea about how to test XOP, then I can add more tests for it too.



================
Comment at: include/llvm/MC/MCInstrAnalysis.h:88
+                                    const MCInst &Inst,
+                                    BitVector &Writes) const;
+
----------------
RKSimon wrote:
> When is it better to use BitVector vs APInt? I don't have an answer but we're incredibly inconsistent on this!
I think using a BitVector (at least in this context) is probably okay. But - to be honest - I don't know the right answer to that question either.
The idea is to use a simple bitvector to do very simple bit manipulation.

APInt has a much richer interface. It allows to do other things (other than bit manipulation). It allows to do arithmetic and logic computation on integers with arbitrary precision. APInt is probably "over designed" for this particular context.

That being said, both interfaces are okay. If you prefer, I can switch to APInt.


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:19
+#include "X86BaseInfo.h"
+#include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/Triple.h"
----------------
RKSimon wrote:
> Include ordering?
Sorry about that. I will move the new include before "X86MCAsmInfo.h".


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:316
+    bool HasVEXOrEVEX = ((Desc.TSFlags & X86II::EncodingMask) == X86II::VEX ||
+                         (Desc.TSFlags & X86II::EncodingMask) == X86II::EVEX);
+
----------------
RKSimon wrote:
> XOP instructions? I think the TBM instructions that use this encoding will be safe as they are always GR32/GR64.
Good point. I forgot about XOP.
However - as you mentioned - I have no idea how to test it.
Do you still want me to add the check for XOP, even if we cannot write a test for it? Alternatively, I can leave a FIXME comment. Not sure what is more appropriate in this context...


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:323
+    const MCRegisterClass &GR32RC = MRI.getRegClass(X86::GR32RegClassID);
+    const MCRegisterClass &XMMRC = MRI.getRegClass(X86::VR128RegClassID);
+
----------------
RKSimon wrote:
> Is this safe for i686 32-bit targets?
It should be safe for i686 because the super-register of a 32-bit GPR is not usable/existent in practice.


https://reviews.llvm.org/D48225