[PATCH] D29874: [X86] Generate VZEROUPPER for Skylake-avx512

[Amjad Aboud via Phabricator via llvm-commits]

aaboud added inline comments.


================
Comment at: lib/Target/X86/X86VZeroUpper.cpp:222
+    // explicitly specified.
+    if (IsCall && !callHasRegMask(MI))
       continue;
----------------
craig.topper wrote:
> aaboud wrote:
> > craig.topper wrote:
> > > Why did this change to not check specific register clobbers?
> > We can get to this point only if ClobberAllTmmReg is true, in such case ClobberAnyYmmReg will also be true, and we will fall-through exactly as we do with this implementation.
> > This was a kind of did code.
> > 
> > Also, can you explain what is unique with "cannot clobber any" and "cannot clobber all" regarding these lines?
> This is probably the first time I've really looked at this code so I think explain what is unique without more thought.
Michael added this code about 3 years ago, he might be able to explain better why he implemented it like this.


================
Comment at: lib/Target/X86/X86VZeroUpper.cpp:225
 
     // The VZEROUPPER instruction resets the upper 128 bits of all AVX
     // registers. In addition, the processor changes back to Clean state, after
----------------
craig.topper wrote:
> aaboud wrote:
> > craig.topper wrote:
> > > This comments is stale with ZMM. It's not upper 128 bits.
> > Actually, this is what it does. even for the ZMM registers it reset the upper 128bits of the YMM corresponding register. Is not it?
> > 
> > The important behavior is the next sentence, which emphasize that processor changes back to Clean state.
> Doesn't it clear the upper 384 bits of ZMM? The comment itself doesn't say YMM. It just says "AVX registers".
I am fine with rephrasing the comment, though AVX registers does not include any ZMM, just YMM.
Do you think I should replace YMM with ZMM?


https://reviews.llvm.org/D29874