Thanks, Craig. LGTM.

Hi Craig, just one comment on the details. Everything else looks good.

I agree with the changes in x86intrin.h and immintrin.h. For the others, I question whether we ought to recommend inclusion of x86intrin.h or immintrin.h. The distinction as I understand it is that immintrin.h is used for Intel-specific intrinsics while x86intrin.h covers all intrinsics for x86-based architectures.

A bit of history: In icc, the f16<=>f32 conversion intrinsics are a bit of an anomaly in that they can be implemented using either native code or emulation code based on the target architecture switch. See https://godbolt.org/g/bQy7xY (thanks, Craig, for the example code). The emulation code lives in the Intel Math Library.

I had actually done that as part of my initial review, so LGTM.

Looks right to me, Craig.

This looks good to me, Craig. I am not worried about the constant folding issue, as I think constant folding these conversion intrinsics (assuming round-to-nearest) is a perfectly valid optimization in the absence of FENV_ACCESS. (FWIW, we don't do this constant folding in icc, but that is only because we have never gotten around to implementing it.) I am also not worried about the spurious 'inexact' exceptions that the changes to the mask intrinsics will cause.

Looks good, Craig. And thanks for the explanation on the 64-bit cases. I had forgotten about the VCVTTSx2USI & VCVTUSI2Sx instructions, which we appear to be generating already.

Thanks, Craig.

Could you please add full context to the patch, Craig?

Thanks, Craig. I noticed it with gcc 4.8.5.

This looks like a nice change.

This looks reasonable to me, Craig. For testing, the obvious thing would be to test the effect on the LoopVectorizer, since it is the primary consumer of this information.

Just one minor suggestion. Otherwise, this LGTM also.

LGTM also.

After some internal discussions, we suspect that Sanjay's counter-example may have undefined behavior according to the C standard.

I'd recommend going ahead with this, Craig. Even if it isn't possible for this bug to trigger a failure today, it looks like a latent bug that should be fixed.

This change seems obviously correct. The diffs in the tests all look like tweaks in the register allocation behavior and aren't testing the behavior you are fixing. Is it possible to write a test that miscompiles due to the existing bug?

LGTM, Craig.

Adding Zvi as an FYI, but I think this is fine to land.

The change looks correct to me modulo Craig's comment, though I suspect he's right that there is a bug in the Intel documentation for FNOP.

Just a minor question on the test changes. Otherwise, this LGTM.

Hi Farhana,

Thanks, Michael. LGTM, again pending @RKSimon's re-review.

Thanks, Michael. This LGTM pending @RKSimon's re-review. But can you please update your sources so that we can see how you merged with https://reviews.llvm.org/D35638, since that will cause conflicts?

Thanks, Farhana. LGTM.

In D34601#810757, @m_zuckerman wrote:

In D34601#808867, @DavidKreitzer wrote:

Hi Michael, I have one small additional comment. Otherwise, this looks good.

This the error I am getting when I try to define the function with a default parameter.
void llvm::scaleShuffleMask(int,llvm::ArrayRef<T>,llvm::SmallVectorImpl<T> &)': could not deduce template argument for 'llvm::ArrayRef<T>' from 'llvm::SmallVector<int,4>'

If you prefer not to touch the CPP file. We can define an overload function in the header file that will explicit set the function signature.

The function in the header will look like:

template <typename t1, unsigned n>
void scaleshufflemask(int scale, typename smallvector<t1, n> mask,
  typename smallvectorimpl<t1> &scaledmask) {
  scaleshufflemask(scale, makearrayref(mask), scaledmask);

This function describe another signature that one of the call in the CPP file uses.

The other option is as I proposed on this review.

What you prefer?

Hi Michael, I have one small additional comment. Otherwise, this looks good.

Thanks for the changes, Michael, and thanks for following up on the perf issue in https://bugs.llvm.org/show_bug.cgi?id=33740.

Hi Michael, sorry it took a while for me to get to this. I was on vacation last week.

Thanks, Farhana. LGTM.

Thanks for the fixes, Farhana. Other than a couple typos, this looks good.

Thanks, Farhana. Please remember to add llvm-commits as a subscriber for LLVM patches. This LGTM unless Simon has additional suggestions.

Not from me. Looks good.

Can we verify how stack is aligned when there is an error code?

Amjad pointed out to me that the incoming alignment to an interrupt handler is only guaranteed to be 0 mod 8, not 8 mod 16 as is the case with the normal x86-64 ABI. HJ mentions this in 26477.

Aligning the stack is certainly the right thing to do. But this isn't the only problem with 26413. I will add a note to the report explaining what I mean.

In D27028#636346, @andrew.w.kaylor wrote:

In D27028#635372, @DavidKreitzer wrote:

When frem has a meaningful result, it is always exact, so perhaps we should omit the rounding behavior argument? I think we still need a constrained frem intrinsic, though, to handle the exceptional behavior in cases such as "frem inf, x" and "frem x, 0".

The implementation is slightly simpler if I give the frem intrinsic a rounding argument, even though it isn't needed. Otherwise, that intrinsic couldn't be handled by the same subclass as the others. I realize that's a fairly weak argument, but I just feel like making this one intrinsic different from the others will make the code ugly.

This all looks pretty good to me, Andy. Regarding

Looks like this was fixed by r288416.

Hi Farhana,
I have no further comments. This LGTM too.
-Dave

Hi Michael,

Thanks for taking care of this, Reid! That fix LGTM.

This LGTM, but I'd like you to also get approvals from Michael & Elena before proceeding.

Hi Farhana,

Hi Farhana,

Just to clarify, the optimal result is movzbl 2(%rdi), %r10d, not

movb 2(%rdi), %al; movzbl %al, %r10d (which is the status quo)

nor

xorl %r10d, %r10d; movb 2(%rdi), %r10d (which is what this patch does)

Correct?

Hi bryant,

The example Aaron sent in email is a good one:

void func(int, int, int, int) __attribute__((no_caller_saved_registers, cdecl));

I have no objection to this solution. I think it is robust in a functional sense. And we can always change it later if we discover that the worst case MOV32r0 sinking scenario is more common than we think. It would give me the warm fuzzies if we had some experimental evidence to confirm the suspicion that this is a rare case. Do you already have that? If not, maybe it would be a good idea to write a late pass that looks for this kind of pattern and count the number of occurrences on, say, cpu2006?

SETcc r8
xor r32, r32 (or mov $0, r32)
movb r32b, r8

And thanks for the performance data! Once this gets committed, I'll have someone run testing on a broader set of workloads.

Hi Michael,

I wasn't suggesting that Marina's fixes should replace this one. Sorry for that confusion. My point was simply that this is just one piece of a more comprehensive solution to the false dependence problem.

I think this is a good change. The cost of inserting an xor when it is not needed is very small. The cost of failing to insert an xor when it IS needed can be huge.

Your question is a good one, and I don't know the answer. Who knows? That might be the cause of all the failures I was getting.

Thanks for the quick response, Michael!

The code change looks reasonable to me Kevin.

Your changes LGTM, Sanjay.

lgtm, Asaf.

Hi Michael,

Thanks for the pointer to https://llvm.org/bugs/show_bug.cgi?id=26776, Reid.

I just have a couple other minor suggestions. Otherwise, LGTM.

Hi Hans,

The connection I saw between removing the _chkstk calls and this transformation is that the pushes will touch the stack in a safe order (starting at %esp, which should be safe, and progressing downwards), whereas the original stores might be in an order that starts by touching an address beyond the allocated stack.

Thanks for the comments, Steven! We are still working on fixing the popcnt false dependence problem, but we are planning to abandon this patch.

Hi Hans,

Thanks, Elena. LGTM.

I think we want to make sure that we move in a direction that makes it easier to do optimizations that affect the CFI between X86FrameLowering and this late pass. For example, we cannot schedule the pushes generated by the X86CallFrameOptimization pass without moving the CFI along with the push. So we generate very poor code in cases like this where the push operands get in the way of outgoing inreg arguments:

Hi Elena,

Thanks for the reviews, Reid and Hans!

Hi Elena,

LGTM too, Kevin.

Thanks for the fixes, Andy.