I have now updated the testcase according to comments by @MaskRay in commit 073cdb239044
Thanks for post-commit review comments.

Updated patch according to review comments.

In D70527#1786718, @rnk wrote:

In D70527#1785552, @ikudrin wrote:

Personally, I would prefer to see the file name and path to be changed as little as possible because that would help to recognize the files better. We cannot use remove_dots() on POSIX OSes to simplify paths, because it may return an invalid path; thus we have to use getRealPath(). If I understand it right, there is no similar problem with the file name itself.

So, which issues this patch is going to solve?

It seems clear to me, the filename could be an absolute symlink to a real file somewhere far removed from the realpath of the parent directory. It seems reasonable that -fdiagnostics-absolute-paths would look through symlinks in this case.

Ping

Update testcases according to review comment.

Thanks!

What about a testcase? It shouldn't be hard to add a small testcase that demonstrate that the changed builtins now work when compiling OpenCL C code for x86. I don't think you have to add all changed builtins to the testcase (but a few to demonstrate the change).

Updated according to review comments

Added a new C++ testcase.
Removed the REQUIRES: avr-registered-target in the avr-builtins.c testcase.

Ping?

Do you think this small lit testcase below work as a testcase for the -ffile-macro-prefix-to-remove option?

I found another reproducer for the problem. This one is slightly different where a different assert blow, but I guess its only a different symptom for the same fault. As this one is slightly smaller you might want to use this one as a testcase instead (or use both)? Run with "opt -bounds-checking -S".

In D48650#1165966, @NoQ wrote:

This commit causes an assertion failure on void foo(int a, int *b) { (int)b < a; }

Run-line: clang -cc1 -analyze -analyzer-checker=core -analyzer-eagerly-assume test.c

So it kinda frustrates RangeConstraintManager with a SymSymExpr that expresses a comparison between a Loc and a NonLoc symbol.

I'll think a bit more on how do i want to address that.

Fixed another case when PHI nodes are not correctly updated.
This time it is when switch is replaced with a single branch.

The fix solves the problem I saw originally in a out of tree target (described in PR37745) but I don't know JumpThreading enough to say if the fix is good or not. Thanks for working on this.

In D47203#1120655, @arsenm wrote:

Shouldn't there be a removePredecessor call somewhere that takes care of this?

Ping?

Ping?

Updated testcases according to comments

In D46379#1118995, @spatel wrote:

In D46379#1118803, @Ka-Ka wrote:

Do you think I should still include the negative testcase that you specified above even if the patch now only affect the GEP case?

Yes - because the earlier revision would have caused a bug with no regression test coverage for it. Let's add that to be safe. Also, I'd prefer to use the utils/update_test_checks.py script on all tests because it makes maintenance easier. And as before, if you check in the baseline version of the test using trunk output, that's even better - we want to keep the tests in place even if the patch gets reverted for some reason.

In D46379#1118168, @spatel wrote:

This is a constant folding change, so the test should be visible with just "opt -constprop ..."

Ie, the test should be minimized and moved under test/Analysis/ConstantFolding.

Updated testcase according to comment.

Adding reviewers.

In D45686#1113426, @lebedev.ri wrote:

In D45686#1113425, @Ka-Ka wrote:

In D45686#1113414, @aaron.ballman wrote:

In D45686#1109295, @dstenb wrote:

Ping.

We have added a lit reproducer for this now in clang-tools-extra: https://reviews.llvm.org/D47251.

The above should be rolled into this patch as the test case verifying the behavioral changes.

I guess the intention from @dstenb was to submit both patches together. The reason for a separate patch with the testcase alone is that the reproducer use clang-tidy and as far as I know all clang-tidy tests exist in clang-tools-extra.

.. and both of them exist in the same svn repo, or git monorepo. (granted, not many use that)

In D45686#1113414, @aaron.ballman wrote:

In D45686#1109295, @dstenb wrote:

Ping.

We have added a lit reproducer for this now in clang-tools-extra: https://reviews.llvm.org/D47251.

The above should be rolled into this patch as the test case verifying the behavioral changes.

Added Jonas Devlieghere as reviewer as he was involved in the review of rL327851 (which seems to be the reason for this patch).

Reduced the size of the testcase.

Adding Matthew Simpson and Sanjay Patel as reviewers also as they have done work in ConstantFold recently.

The reason for me to use WeakTrackingVH instead of WeakVH from the beginning was only that it was used elsewhere in the file. However as you point out WeakVH should be fine as RAUW is not used in RecursivelyDeleteTriviallyDeadInstructions(). I adjust the code to use WeakVH.

Ping?

Ping!
Anyone up for reviewing this?

This definitely solve the problem I saw in the issue pr37369.

Changed testcase according what was suggested by lebedev.ri

Ping!
Anyone up for reviewing this?

Adding Matthias Braun and Krzysztof Parzyszek as reviewer as it seems you both have done work in BranchFolding.cpp before. Do you have time to review this change?

In D46048#1079135, @jonpa wrote:

Adding Uli as a reviewer for the test case, since it's SystemZ (he always reviews SystemZ changes). Note: as Kalle explained to me in a mail, this test case has been somewhat modified to expose the bug on SystemZ, but since this is a .mir test case which passes the machine verifier, I think this is not a problem.

Ping.
As I said before the change LGTM, but I would prefer if someone else also reviewed this change.

The change LGTM, but I'm not sure if I'm allowed to set "ready to land". I would prefer if Adrian Prantl or Reid Kleckner also reviewed this change (both now added as reviewers).

Rebased

In D38229#884660, @aprantl wrote:

Could you please file a PR about this?

In D38229#880149, @rnk wrote:

One concern I have about this is that it may lead to inaccurate debug info when the value is computed long before its (dead) assignment. Consider this kind of program (untested, yet):

int v1 = getval();
int v2 = getval();
usevals(v1, v2);
v1 = v2;
if (v1) { /* empty after inlining, making v2 dead */ }

After optimization, v1 will probably use a CSR and v2 will use a volatile register clobbered by the usevals call. So, the dead v1 = v2 assignment will produce a DBG_VALUE of a physical register that has been clobbered. It would be innacurate to hoist the DBG_VALUE across the usevals call, since the value should still be the result of the first getval call, not the second.

One way to address this would be to avoid moving DBG_VALUEs across instructions with different DebugLoc lines.

Even if we decide that this issue is unimportant, we should probably write tests for it.

When I tried your test program above I found that faulty hoisting of a DBG_VALUE across the call to usevals seems to be done by the CodeGenPrepare pass (before instruction selection). This must be a bug. If you want I can convert code above into a testcase but it will contain this faulty hoisted DBG_VALUE from the CodeGenPrepare pass. This patch will not impact that testcase.

Minor fixes.

In D38229#880149, @rnk wrote:

One concern I have about this is that it may lead to inaccurate debug info when the value is computed long before its (dead) assignment. Consider this kind of program (untested, yet):

int v1 = getval();
int v2 = getval();
usevals(v1, v2);
v1 = v2;
if (v1) { /* empty after inlining, making v2 dead */ }

After optimization, v1 will probably use a CSR and v2 will use a volatile register clobbered by the usevals call. So, the dead v1 = v2 assignment will produce a DBG_VALUE of a physical register that has been clobbered. It would be innacurate to hoist the DBG_VALUE across the usevals call, since the value should still be the result of the first getval call, not the second.

One way to address this would be to avoid moving DBG_VALUEs across instructions with different DebugLoc lines.

Even if we decide that this issue is unimportant, we should probably write tests for it.

In D37428#861140, @dblaikie wrote:

Not sure what you mean by "avoid emitting unnecessary stop points" - do you have a test case for that?

I updated the testcase according to what David Blaikie suggested.
I also rewrote to code to be a bit more robust and avoid emitting unnecessary stoppoints.

I had to revert this change due to a buildbot failure. I'm trying to recreate the fault locally ...

Updated the testcase "run line"

Updated the testcase to use FileCheck.

Any news about this issue?

In D27983#628114, @qcolombet wrote:

Hi,

The change looks good, but do you have a test case to add with it?

Thanks,
-Quentin

I have a reproducer for an out of target backend. I will give it a try (tomorrow) to transform it into a x86 reproducer, but I suspect that it might be hard to get it working.

This patch solve the problem I have in the "out of tree" backend I work with. Many thanks!