Thanks Eli!

Ping. @Gerolf, are you happy with the update?

You can use something like the command below to get a comparison between 2 sets of runs. Also, doing single runs only will probably result in quite noisy results.

I've verified this does not change CodeGen for the test-suite + various external suites. Slightly positive impact on compile-time (-0.1 % geomean speedup for test-suite + SPEC & co, with -O1 on X86)

Remove unused MarkDirty.

In D60266#1455299, @efriedma wrote:

Why do we need to rotate the loop before unrolling? llvm::UnrollLoop currently refuses to unroll loops where the latch is an unconditional branch, but that isn't a fundamental limitation, as far as I can tell. We already support unrolling loops where the latch is not the exit branch; allowing loops where the latch doesn't exit at all is a minor extension. Granted, it might be more efficient to explicitly rotate the loop before unrolling, so we don't clone quite so much code.

As a way to test this on a larger number of cases, could you add it to the default pass pipeline (maybe at multiple points) and bootstrap clang with it (or build the test-suite & co)?

LGTM, it would be great to have Cortex-M4 use the MachineScheduler in the non -Oz cases. Lets keep track of the issues (I expect more to surface) and go from there.

The change in the AArch64 assembly looks good to me.

@sanjoy it would be great if you could have a quick look and check if the new approach makes sense to you.

Thanks for the excellent suggestions! I've updated the text to use the GEP based description, as the GEP documentation spells out the behavior with respect to invalid pointers in detail. Also incorporated Hal's comments.

Thanks!

Ping. Ayal, does this address your comments appropriately?

In D59065#1501553, @jdoerfert wrote:

In D59065#1501459, @fhahn wrote:

• What happened to the poison result sentence. I think we need that to ensure the "based on the same ..." part. Otherwise, we can butcher the pointer with the mask and end up with a "valid pointer" into a different object. I might be wrong here, just want to make sure we have thought of it.

My understanding from Hal's comments was that the behavior should be clear from the pre-existing semantics, but I might have missed something?

I don't see why but I might be wrong (@hfinkel). What I thought is detailed below.

The case where we fail to clear bits that must be zero should be clearly handled by the existing semantics.

Sure.

The case where we might clear meaningful bits

I'm not sure about this. At least I think this is not what you intended to do initially but maybe this is OK for you.

In D59065#1500576, @jdoerfert wrote:

In D59065#1497965, @fhahn wrote:

In D59065#1497227, @jdoerfert wrote:

What happens if I use an integer type bigger than the native pointer width? Or smaller?

Initially I wanted to restrict the mask type to the native pointer width, but couldn't find how to do that in Intrinsics.td. On second thought, this might be more restrictive than necessary. I've adjusted the wording to make it clear that we zero-extend or truncate the mask if the bit width does not match the pointer size of the target. What do you think?

The wording makes it clear now (what is supposed to happen at least).

This looks like an interesting case! I think there are 2 things going wrong (see input to the scheduler below)

ping

Ran clang-format.

ping

LGTM. I've also added a few other reviewers, in case they have additional thoughts.

In D59065#1497227, @jdoerfert wrote:

What happens if I use an integer type bigger than the native pointer width? Or smaller?

Thanks for your feedback @jdoerfert & @hfinkel! I've adjusted the description as suggested. I dropped the information about 'relevant' bits and made the description more precise.

Change behavior from undefined to poison when passing masks that zero out relevant bits of a pointer, thanks @aqjune

thanks!

In D61669#1495274, @jdoerfert wrote:

I'd look into this but I have trouble locating the "ptrmask" intrinsic in LLVM/Clang/LangRef. Could you help me out?

In D60266#1455299, @efriedma wrote:

On a related note, independent of what we do in unrolling, it would probably be worthwhile to teach loop rotation to rotate loops where the cloned header would fold to zero instructions.

Add missing attributes to @init argument.

I went with adding a pointer-mask intrinsic, as suggested.

ping

In D59918#1469666, @jdoerfert wrote:

In D59918#1467443, @fhahn wrote:

And finally, what can we do to check correctness? As the current attribute definitions are a bit fuzzy, it seems this could lead to problems down the road, in case we infer invalid attributes, which only get used (and cause problems) a bit later. Assuming we have a clear definition of a set of attributes, how hard would it be to verify the attributes in a module (maybe we are doing it already)?

I'm unsure I understand this question. If a fixpoint is reached, as reported by the AbstractAttribute::updateImpl methods, we should have derived "correct" information. Obviously, there might be a bug in the logic of the AbstractAttribute::updateImpl function, or the associated materialization, the dependence tracking in the attributor, etc. but I do not have a better solution than more (stress) tests (see D59903). I open a few bugs while I was developing attribute deductions in this framework and compare the results to the existing one. We current derive different things we should not as described in PR41336 and PR41328. More attributes will also trigger, or better expose, "bugs" down the line (see D59917) and we will probably have to manually look into the root cause as they pop up, was the attribute not correct or later reasoning based on it.

Use InstBB instead of I->getParent(). Thanks, I was looking at the wrong line :)

In D61300#1493707, @jdoerfert wrote:

Do we need to "repair" the folded branch or could we also determine earlier that the current block is dead (it will have an unconditional branch to a dead block, right?) and avoid folding?
It might make sense to fold due to the DT update though.

ping

Filter operands for getIntrinsicCallCost as well.

In D59995#1455952, @Ayal wrote:

The culprit here is the assumption made by TTI.getOperandsScalarizationOverhead(Operands, VF) that all its Operands will be vectorized according to VF, and would thus require extraction to feed a scalarized/replicated user. But any such Operand might not get vectorized, and possibly must not get vectorized, e.g., due to an incompatible type as demonstrated by PR41294 and the testcase. In some cases an Operand will obviously not be vectorized, such as if it's loop-invariant or live-in. More generally, LV uses the following:

auto needsExtract = [&](Instruction *I) -> bool {
  return TheLoop->contains(I) && !isScalarAfterVectorization(I, VF);
};

which would require passing not only TheLoop into getScalarizationOverhead(I, VF, TTI) but also the CM --- better turn this static function into a method of CM?

Split out moving various functions to LoopVectorizationCostModel to D61638.

Updated increments.

Remove restriction for single use PHIs. We can replace such PHIs with their incoming
value, iff all uses are in the loop exit block or in the outer loop header, if
the incoming value is defined in the inner loop header (reduction phis).

Add test case without loop exit, clang-format-diff

In D60133#1452430, @niravd wrote:

It's not sufficient to check if you can merge two stores into a valid node; there are backends where you need 4 or more to get a legal merged store.

If you look at target-specific implementations of CanMergeStoresTo it essentially serves as a context-specific find maximum store which is what we need here. If you massage that interface a bit you can fold most of this check in there.

Limit this patch to visitTokenFactor, limiting the number of operands to inline to 2048 nodes.

In D60690#1469460, @Gerolf wrote:

Could you separate size one checks are OK vs. checks that require even powers of 2? That smells like an inconsistency worth calling out explicitly.

Push even element checks to the impacted functions.

In D61397#1487955, @niravd wrote:

Notes inline, but I think the majority of the compile time improvements come from keepign TokenFactor operand counts bounded. This should be changed to do reflect that.

Relax limits to 1000 nodes to explore. Further experimenting showed that those
bigger limits are still sufficient to ensure limiting quadratic compile time.

As per feedback here & on llvm-commits it seems like this is not the right thing at the moment. Thanks for taking a look!

Thanks for taking a look so quickly! I'll look into how to best set-up DYLD_LIBRARY_PATH in that case. I am after a more lightweight solution than CLANG_ENABLE_BOOTSTRAP, i.e. creating/configuring the separate build stages manually.

I plan to add this on top of the integer range support patches, abandoning this for now