Duplicate - D87145

@foad, @RKSimon, Thanks for directing me to the other patch.
I'll just close this and track D87145.

In D74484#1980606, @spatel wrote:

Sorry, I did not explain my previous comment as well as possible. Let's take this minimal example:

define i1 @f(i8 %a) {
  %conv = sext i8 %a to i32
  %cmp = icmp slt i32 %conv, 109
  ret i1 %cmp
}

As per @spatel 's request,
Added a comment explaining the motive behind implementing the transformation in aggressive instcombine instead of normal instcombine.

In D74484#1966744, @spatel wrote:

In D74484#1963795, @aymanmus wrote:

@spatel: Basically the test cases with the real motivation are the first 4.

Do you mean the first 4 with diffs, or the first 4 that are being added as new tests?

The first 4 with diff.

Where most of the other cases are there to check various "edge" cases of the added code behavior.
I think, that even if some of the tests are not in a canonical form (and can be optimized by instcombine), we still have an added value having them here in order to check the behavior of this specific pass with similar cases.
Don't you agree?

Yes, I agree that we want to have tests for edge cases to make sure that the logic is correct here. But we also should have tests that show why this patch is necessary - functions that could not be solved in regular instcombine easily.

I agree with you 100%. That's why we have several test cases.

This change has broke the update_llc_test_checks.py tool's functionality for any test that includes a local function.
The tool now removes all "; CHECK" lines from such tests and emits nothing instead.
You can check this by adding "dso_local" attribute to any of the X86 existing tests (for example /llvm/CodeGen/X86/add-i64.ll) and run "update_llc_test_checks.py on it.

@spatel: Basically the test cases with the real motivation are the first 4.
Where most of the other cases are there to check various "edge" cases of the added code behavior.
I think, that even if some of the tests are not in a canonical form (and can be optimized by instcombine), we still have an added value having them here in order to check the behavior of this specific pass with similar cases.
Don't you agree?

Just checked the patch @spatel mentioned.
Doesn't really help with the cases provided here.

PING.

Ping #2.

Ping.

Fix the constant aux functions to correctly handle vector constants.

In D74484#1873830, @nikic wrote:

@aymanmus I have something along these lines in mind...

%conv = sext i8 %a to i32
%conv2 = sext i8 %b to i32
%conv3 = sext i8 %c to i32
%cmp = icmp slt i32 %conv3, TOO_BIG_CONST
%cond = select i1 %cmp, i32 %conv2, i32 %conv
%conv4 = trunc i32 %cond to i16
ret i16 %conv4

That is, where the icmp has a form that passes the isConstOrExt(C->getOperand(0)) && isConstOrExt(C->getOperand(1)) check, but later gets rejected due to constant bitwidth.

Not sure if that example does it, but I think there must be some case like this, unless I'm misunderstanding the patch.

Addressed @nikic & @lebedev.ri comments.

In D74484#1873745, @nikic wrote:

I think this is missing a test-case where the select could be truncated if you ignored the icmp, but not if you require the icmp to also be truncated. That should show up as a regression in the test diffs.

Deal with APInt instead of 64-bit value in constants.

In D74484#1873705, @lebedev.ri wrote:

In D74484#1872137, @lebedev.ri wrote:

This seems not generic enough to me, can't we not require the icmp operands to be constants/[sz]ext's,
but instead try to see if it can be evaluated in smaller bitwidth (what the rest of the code here does)?

As long as the icmp can be shrunked to at least as small bitwidth as we need there to get rid of cast,
i think we can always pick the actual bitwidth we'll use, which might be wider than minimal?

After thinking about this, i'd be okay with not doing that straight away.
It should be doable, but would require substantial redesign to nicely model more than one dag and their connection/dependency.

Extract the std::max omission to a separate patch as requested (patch D74476)

Adding full context to patch.

Removed all cmp instruction handling to get a pure SELECT patch support.

Following @nikic's comment:
I agree, the patch is missing some select exclusive tests.
Adding such test cases.
IMHO, there is no added value to splitting to 2 patches, the select changed are too obvious, and the compare is strongly related to the select changes.

Fix @spatel's test case. Thanks for the review.
Involve constants in cmp operations in the MinBitWidth calculations.

Somehow the test wasn't included with the differential text file.
Adding now.

Thanks Anton.
Can you please keep us updated with any progress in this regard?

Just to make sure I understand the change correctly:
This can still hoist instructions outside of their BB and execute them speculatively even if you can't prove that all possible paths execute the instruction, am I right?

Hi Anton,

Targeted Gadi's comments.

Manually reducing "bugpointed" test.

Adding triple & a test case for PR23657

Ping #3!

Ping #3!

Abandoning - lack of response.

Minor comments but LGTM overall.

Kind ping #2

Kind ping #2

The bug was filed for Clang's 3.8 version, it passed with clang-4.0 and clang-ToT (top of trunc).
Seems like the issue was fixed while ago.

The goal is to make tablegen generate the "correct" tables (according to what we define is correct).
So it's up to us to decide what will go in and out of the tables.

@craig.topper that's right, at the end of this process both the generated and manual tables will hold the same entries. But the backend would be disabled and we would include a copy of the last generated inc file from it.
The main challenge here is how to integrate the TableGen's backend and taking advantage of its capabilities while supervising and validating each change made to them.

No it won't completely replace it. It would be used as a reference to the manual tables.
It's explained in the description section of this patch.
The decision was made after a discussion in the community's mailing list (http://lists.llvm.org/pipermail/llvm-dev/2017-July/115734.html)

Ping.

Ping.

LGTM

Fixed in D38120

Since the backend would be disabled at the end of the process, I think that it's better to keep it that way from the beginning.
The generated tables do not mean anything now and IMHO should not be visible to anyone who has no interest in contributing to this specific effort.
So I guess the best way to keep the temporary tables "hidden" is by enabling the "run-by-demand" mechanism now.

Total redesign to the previous solution.
moved the transformation to a more "natural" area.

Thanks for the answer.
LGTM