ping

rebase over a landed set of tests

Update ascii art to reflect change and clarify a confusing bit in the original.

Realized while working on another patch that this one is incomplete.

In D94542#2500145, @MaskRay wrote:

The value of -mbranches-within-32B-boundaries also dilutes over time (it mitigates issues for some Skylake architectures).
(Surprising to me, this feature has been available for one year but I don't see a lot of adoption).

The performance impact of the microcode fix were highly variable depending on the exact details of each workload. Mostly barely moved, some really suffered. I know of a couple of organizations using the mitigation, but I agree with your general point about this being something that decays in value with time. Hopefully, in a few years, we can stop talking about this.

In D94542#2500116, @dblaikie wrote:

In D94542#2500058, @reames wrote:

... is pretty important that turning on debug info does not change the generated machine code - otherwise there's the chance of heisenbugs.

If this is a case where enabling debug info changes the selected machine code, that is a bug and one worth fixing one way or another.

David, thanks for chiming in here. Reading your wording made me realize I was wrong in my take here. I had been thinking of this as a debug info quality problem, but you're right, that's not what is actually going on here.

I'm not convinced that disabling these optimizations is warranted. I'm not actively opposing the patch, just want to list my concerns for the record.

LGTM w/an MIR test case added before submission.

In D94542#2494838, @MaskRay wrote:

In D94542#2494774, @reames wrote:

I also ask that a bit more background be given to justify this change. I found the bug (https://bugs.llvm.org/show_bug.cgi?id=42138#c13), but that gives no information about the cause of the assembly difference. Has anyone examined the cause of the labels being emitted in debug mode to see if they're necessary/useful?

I explained the cause in the description: "A -g build has more MCSymbol's and therefore may have different assembler output (e.g. a MCRelaxableFragment (jmp) may have 5 bytes with -O1 while 2 bytes with -O1 -g)."

This doesn't get at the root cause though. Are those labels doing anything in the debug build? Why are they emitted? Can they be reasonably removed?

This review has been closed for nearly 10 months. Please move discussion of proposed changes to the bug or another relevant location.

Both the align and branch handling are "optimizations". I object to one being enabled and the other disabled. If you want them both on by default, fine. If you want them both off by default, fine. Having one off and one on is confusing.

In rG4b33b2387787aef5020450cdcc8dde231eb0a5fc#969612, @markus wrote:

This commit seem to cause

$ opt -verify -loop-vectorize bbi-51639.ll -S
opt: /repo/elavkje/llvm-project-upstream/llvm/lib/Transforms/Utils/LoopVersioning.cpp:47: llvm::LoopVersioning::LoopVersioning(const llvm::LoopAccessInfo &, ArrayRef<llvm::RuntimePointerCheck>, llvm::Loop *, llvm::LoopInfo *, llvm::DominatorTree *, llv
m::ScalarEvolution *): Assertion `L->getExitBlock() && "No single exit block"' failed.

bbi-51639.ll852 BDownload

Fixed in commit caafdf07b. This was a spurious assert.

rebase and ping

LGTM w/changes.

Side note: The -1/+1s appearing everywhere confused me until I looked into the getConstMetaVal function. Having the index returned be the second in the tag, value pair seems slightly confusing, but it fits the existing structure.

LGTM as a testing aid. Should be removed once invoke support is stabilized and fully enabled.

Also LGTM. Thanks.

I would strongly prefer that instead of simply asserting that we retie the registers. Doing so should be fairly simple - we just need to keep track of which register pairs we untied for the possible fold, and use a lambda to retie them.

I'm dropping my request which triggered this review. Feel free to close.

Context: This patch was reverted following failures in stage2 builders (only). Still investigating root cause. Reproduction is a bit of a pain.

rebase over landed tests

Add requested test.

Address stylistic updates. Test changes to be done separately in near future.

In D93906#2477485, @nikic wrote:

In D93906#2476763, @reames wrote:

In D93906#2476680, @nikic wrote:

Isn't this already covered by the zero exit count optimization in IndVars?

No. I believe you're referring to optimizeLoopExits. That does not modify the CFG, it just folds conditions to constants. This will specifically modify the CFG and remove the loop.

Yes, that's the transform I had in mind. I see the difference now, but I'm not sure I understand the larger picture / motivation behind this. The true/false branches will get folded away by SimplifyCFG -- so is this a phase ordering problem where SimplifyCFG runs too late (if so, can we add a PhaseOrdering test)? Should we also try to fold away (to unreachable) dead exits, rather than just dead backedges?

You can view this as a phase ordering issue, but really I see it as "this pass is supposed to delete dead loops" + "a case came up in discussion which we'd missed" + "easy no-cost fix".

In D93906#2476680, @nikic wrote:

Isn't this already covered by the zero exit count optimization in IndVars?

No. I believe you're referring to optimizeLoopExits. That does not modify the CFG, it just folds conditions to constants. This will specifically modify the CFG and remove the loop.

ping

ping

In D93764#2473241, @fhahn wrote:

I think this is a separate issue (the condition is dependent on the induction variable), but nevertheless interesting to pursue. IIRC there is already a pass that tries to do something similar, but I don't know which one of the top of my head. Will check.

This is InductiveRangeCheckElimination which implements a form of iteration set splitting. I'll warn that IRCE has been historically bug prone, and is not on by default upstream.

Ayal, Florian, thank you for the efforts on this review. I really appreciate the active review.

ping now that previous patch in series has landed

LGTM

In D93317#2470271, @fhahn wrote:

In D93317#2468673, @reames wrote:

Florian, any other edge cases you can think of? I'd completely missed that one. Thank you for finding it!

I think there are some scenarios when we break some LCSSA PHIs that use PHIs created during SCEV expansion when we generate the runtime checks. After a first glance, it appears like an issue after we add the conditional branch from the middle block. The example below should cause a verifier failure with opt -loop-vectorize -force-vector-width=4. I tried to reduce it a bit, but unfortunately it is still quite ugly.

I can confirm the failure and will debug. Thank you again for finding a cornercase.

As implemented, this is a bit too specific to one particular pattern to be commitable, but I think you've got the seed of a good idea here.

Reading through the code, I had a couple of separate thoughts. If you're interested, feel free to follow up on these. If not, feel free to disregard.

Incorporate a fix for the issue Florian found. Essentially, we can't both treat single use exit conditions as dead, and allow predication to use them in a mask. Since we know they're evaluate to true for the entire vector body, we can simply ignore them when forming edge predicates.

Ok, I see what's going on w/Florian's example. It's an interaction with block predication and early exits. Essentially, block predication expects to be able to add uses of any condition in the loop, and the dead code elimination has eliminated the exit condition in the vector loop. I can restrict the legality slightly to avoid this interaction easily enough, but I want to think a bit first if there's a clean integration.

JFYI, multiple latch tests added in f106b2, but they don't impact the diff as SCEV can't prove exit counts and thus we don't vectorize.

Incorporate wording/style comments from Ayal, test rebase pending.

Ayal, thanks for all the great wordsmith comments!

ping

One further simplification enabled by previous rebase.

Rebase over a81db8b31. While that change is NFC, the code structure makes it much easier to disable vectorization when predication is demanded and we can't provide it.

Update tests to cover cases where we can't vectorize due to either a) size, or b) predication.

rebase over landed tests

LGTM.

Address review comments.

As a side note, I don't think you actually need to disallow conversion of the null pointer between two different NI address spaces. If you wanted to improve opt quality, folding your check into the previous one would be beneficial. I'm not going to require that as this a) fixes a correctness bug, and b) has been outstanding for way to long.

Hard reject. This patch will not be accepted. This is a major design change.

Add non-zero restriction. For now, restrict to constant case. I could have used isKnownNonZero, but that would require recursing through both operands which I'd rather not do from a compile time perspective.

LGTM. I had to grab a sheet of paper and run some examples to convince myself that your code worked - even with the explanation - but I got there eventually. :)