Adding some people who might have opinions on more FP intrinsics.

isSized() should return true for these structs. They are sized for our definition of sized.

I think this (removing GlobalCleanupPM) makes sense at a high level, but this is a pretty substantial pipeline change and will need more thorough evaluation (e.g. looking at IR diffs on test suite). For example, it is not at all obvious to me that moving InstCombine before EarlyCSE is harmless, given the impact this may have on one-use heuristics.

I've put up https://reviews.llvm.org/D146813 to move the loop invariant GEP reassociation into LICM, which should allow us to drop the LoopInfo dependency from InstCombine.

@mkazantsev Thanks for the context! In that case I'll go with this patch, as it seems like we are already disallowing this everywhere apart from this one fold, so it makes more sense to be consistent.

@uabelho Should be fixed by https://github.com/llvm/llvm-project/commit/4923b4dbaca386b532c80f0572c2d6a0547a2a3d.

Minimal test case:

define internal ptr @callee(ptr %dead) {
  ret ptr null
}

LGTM

LGTM

@nickdesaulniers I don't think we want to handle that on the LLVM side. That will be fundamentally unreliable. If Clang wishes to make "recoverable" ubsan have arbitrary but well-defined behavior in the case of UB, it needs to generate appropriate IR to model that. For example, instead of generating

if (x == 0) {
  report_div_by_zero();
}
res = y / x;

it needs to generate

if (x == 0) {
  report_div_by_zero();
  res = 0; // or any other value
} else {
  res = y / x;
}

And similarly for other cases.

I think this is right, but I'm not very confident about it. The problem is that mergeicmps can reorder loads, but I think these isolated comparisons will not get reordered before other ones (only possibly after), so I think it's fine. But maybe I just didn't manage to construct the right test case.

LGTM

LGTM

LG

LGTM

Thanks for the PhaseOrdering test, I think I get the problem now. ArgPromotion is the first pass in the CGSCC pipeline, and works a bit unusually in that promotion will insert loads in the caller (outside the current SCC). This means the we run ArgPromotion on a child function, which inserts instructions in the parent and simplify the child. Then we run ArgPromotion on the parent, but at this point the parent has not been simplified yet. As such, it is important for ArgPromotion to clean up instructions it inserts. There's possibly some phase ordering improvement we could make here, but I don't see anything obvious. (Just moving ArgPromotion to the end of the CGSCC pipeline would cause the reverse issue that function simplification can not make use of the promotion for cleanup.)

This looks good to me, but could you please pre-commit the new tests with baseline check lines, and then rebase this patch so that only the test diff is visible?

LGTM

LGTM

As @RKSimon said, this transform is in the wrong place. It has nothing to do with shuffled intrinsic operands...

@jdoerfert Yeah, there is a pending patch that fixes this for !range here: https://reviews.llvm.org/D142687 We'll need similar changes for nonnull/align as well.

Could you please also add tests for the !noundef case? (Preferably one with the first load noundef and one with the second. I'm not sure you're checking the right one currently.)

Implementation looks fine, but this needs more test coverage.

LGTM

LGTM

Proof: https://alive2.llvm.org/ce/z/VNNPeG

LGTM

Nearly there, I think...

All of the test changes are, to my eye, semantically the same.

LGTM

LGTM

LGTM

It looks like this broke the expensive checks build (x86-64-baseptr.ll and i386-baseptr.ll fail).

Upload correct patch.

Improve comments.

LGTM

Can you please rebase over https://github.com/llvm/llvm-project/commit/d0de2c51c9a9fc0fedb97ee98f61ce08cb34972b? I hope that will clarify what the actual change here is, because making foldOperationIntoSelectOperand() fallible doesn't make a whole lot of sense.

In D146423#4208625, @mkazantsev wrote:

Actually it was I who proposed Dmitry to split it off into a separate analysis. The main advantage of this approach is that we can then create some static analysis tools that don't necessarily modify the IR to give you the same info.

LGTM

LGTM

LGTM. Assuming you don't have commit access, can you tell me the Name <email> to use for the commit?

FWIW, I am quite unhappy with the implementation quality of this pass, but I don't think I have the energy to deal with this. In the future, due diligence for pass enablement needs to include a review of the pass implementation by a domain expert, if this was not already done as part of the initial implementation. (Domain expert = SCEV reviewer in this context.)

In D146424#4206466, @tlively wrote:

Thanks! I didn't know about this functionality. Is there a generally accepted mechanism to mark intrinsics as experimental so that there is no expectation that they will be auto-upgraded like this?

Using max exit count is incorrect. The computed max may be larger than the actual trip count of this exit.

Change looks fine, but is missing a test case.

ping ;)

I haven't looked in detail what you're doing here, but you're clearly looking for the ConstantRange class. makeExactICmpRegion(), add(), sub(), binaryNot(), etc.

In D144445#4204759, @nikic wrote:

Extracting the stride from GEPs turned out to be trickier than I expected...

LG

Extracting the stride from GEPs turned out to be trickier than I expected...