Thanks for the quick review!
Will commit this version.

Thanks for the test case!
The problem is:

The fix has been committed as f35bcea1d4748889b8240defdf00cb7a71cbe070.

Thanks for the report!
I guess function searchALUInst should also check overlapped register usage of DestReg.

Check the dead EFLAGS def directly instead of calling computeRegisterLiveness.
Add implicit dead def EFLAGS to new alu instructions.

In D101970#2779338, @davezarzycki wrote:

Actually, it is more complicated, and you're right that it also can involve overflow. I'm not sure if this is a trivial optimization when dependent comparisons are involved.

Thanks for the report!

Thank you, Sanjay.
It really fixed our problem in a larger application.

This patch generates wrong code for the function

void foo(char *p, int l, int t) {
  *p = static_cast<char>(static_cast<char>(t << (8u - l)) |
                         static_cast<char>(0xffu >> l));
}

In D101970#2750517, @xbolva00 wrote:

Can you try this patch with http://llvm-compile-time-tracker.com/ ?

In D101970#2742388, @lebedev.ri wrote:

I think it is good when the patch's description not just what the patch does, but also why it does that.

Thanks for the improvement.
Will commit this version.

Reformat the patch.

In D98899#2640159, @nikic wrote:

This looks reasonable to me. Dropping nonnull is not strictly necessary (because it has no immediate UB), but doesn't hurt either.

I was going to suggest using AttrBuilder to drop all attributes at once, but I'm not sure that works with dereferenceable attributes.

Compile the following cpp code

ANY_EXTEND of SEXTLOAD/ZEXTLOAD should not be allowed.
In theory if both loads have same SEXTLOAD/ZEXTLOAD, we can change ExpOpcode to corresponding SIGN_EXTEND/ZERO_EXTEND, but it will make the code more complex, and this case should be extremely rare.

ping

Check conflicting extload.

Add support for ISD::VSELECT.

Rebase the test case.

Add more test cases.

Rebase the code.

In D94851#2507869, @lebedev.ri wrote:

In D94851#2507867, @Carrot wrote:

Thanks for the info.

I will discard this patch and reverse the pattern at function CodeGenPrepare::optimizeExt.

Not really sure why that would go into CodeGenPrepare, which is a temporary hack,
DAGCombine is the usual counterpart.

Thanks for the info.

Is this a general problem with MBP then? I haven't noticed it before this optimization, but maybe it was just luck or not investigating the right examples. I'm trying to understand where is the best place to fix it, and if it's even feasible.

@GalZohar, thanks for the example, now I understand your problem. The new layout does increase the number of executed branches for the path BB2->BB3->BB4->BB2. Unfortunately most of the current MBP algorithms don't consider this factor. When considering the number of fall through only, the new layout has more fall through and less taken branch.

@GalZohar, without a testcase I can't say what's the problem.

@GalZohar, the various layout algorithms in MachineBlockPlacement mainly consider the number of fall-throughs and dynamic number of branch instructions (usually they are consistent) according to branch probabilities. So you can try to build your application with profiling. Or you can compile this file with -Os since the improvement in this patch is disabled with -Os.

The real root cause of crash in building chrome and firefox has been committed https://reviews.llvm.org/D92882.

Will commit this version.

In D92882#2444285, @davidxl wrote:

It is possible to synthesize a test case that rotates the entry without the fix, no? The expected output is that the entry is not rotated (no need to reproduce crash).

In D92882#2441215, @davidxl wrote:

yes, a test case please :) Any performance numbers?

In D92882#2441182, @snehasish wrote:

Can we add a test for this?

@RKSimon, thank you for your help. I do have another test case that has bad vectorization with scatters, https://bugs.llvm.org/show_bug.cgi?id=48429.

@fhahn, thank you for the clarification of the isLegalMaskedGather and getGatherScatterOpCost usage.

In D89088#2348847, @dmajor wrote:

In case it helps, I turned on debug logging for this pass on our repro: https://paste.mozilla.org/HBDkeGXO (starts at line 60)

It has been reverted.

@aeubanks, can you extract a test case from your failure?
It will be very helpful for me to debug the problem.

Then it should be an unknown bug in MBP.

As jmorse has pointed out, the function layout is incorrect at the assertion time.

@dmajor, without a test case it is difficult to point out what's wrong since there is no obvious relation between this patch and VarLocBasedImpl.cpp. I will try my best to do some simple analysis/guess:

For the attached test case, it can cause bad layout, block .problem is placed under .cold. With this patch .problem is optimally placed after .pred.