This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Model first exit values using VPLiveOut.
ClosedPublic

Authored by fhahn on Apr 11 2022, 12:52 PM.

Details

Summary

This patch introduces a new VPLiveOut subclass of VPUser to model
exit values explicitly. The initial version handles exit values that
are neither part of induction or reduction chains nor first order
recurrence phis.

Fixes #51366, #54867, #55167, #55459

Diff Detail

Event Timeline

fhahn created this revision.Apr 11 2022, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 12:52 PM
fhahn requested review of this revision.Apr 11 2022, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 12:52 PM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added inline comments.Apr 13 2022, 1:17 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8900

This stretches the already too long method to exceed 400 lines... please outline.

8907

This should get rather than add, right?

Check if V is a header phi recipe, whose exit values are not represented yet?

Exit values of inductions are disconnected from the loop body by precomputing them in preheader, so modeling them as VPInstructions in the middle block would require also modeling their precomputation in the preheader?

8914

Can alternatively first identify all VPValues in loop which are Not-To-Feed-Exit-VPInstruction, then wrap every middle-block loop-closed phi with an Exit VPInstruction provided it does not use an NTFEV VPValue? Instead of collecting NTFEV's and calling removeExitUsers() for each.

llvm/lib/Transforms/Vectorize/VPlan.cpp
828

Indicate that ExitValue usesScalars()?
Perhaps worth emphasizing in the name - reduction epilogues using vectors probably deserve a separate "Exit Value" recipe.

832

nit: empty line?

fhahn edited the summary of this revision. (Show Details)Apr 21 2022, 1:14 AM
fhahn updated this revision to Diff 424740.Apr 23 2022, 11:51 AM

Address latest comments, thanks!

fhahn marked 5 inline comments as done.Apr 23 2022, 11:58 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8900

Moved to a separate function, thanks!

8907

This should get rather than add, right?

We could have loop invariant incoming values which won't be modeled in VPlan, hence getOrAddVPValue.

Exit values of inductions are disconnected from the loop body by precomputing them in preheader, so modeling them as VPInstructions in the middle block would require also modeling their precomputation in the preheader?

We should be able to add those to the pre-header, but I think it's best to do it one step at a time?

8914

Updated to collect values to skip first, thanks!

llvm/lib/Transforms/Vectorize/VPlan.cpp
828

Indicate that ExitValue usesScalars()?

Added usesScalars implementation.

Perhaps worth emphasizing in the name - reduction epilogues using vectors probably deserve a separate "Exit Value" recipe.

Good point, not sure what the ideal name would be. Changed ScalarExitValue but I am not sure if that is much better. They model all exit values that do not need special handling.

832

Removed, thanks!

fhahn updated this revision to Diff 424742.Apr 23 2022, 12:00 PM
fhahn marked 5 inline comments as done.

Bring back getOrAddVPValue, as per inline comment.

Ayal added inline comments.Apr 25 2022, 3:15 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4107–4108

Is this made redundant by ScalarExitValues?

8755

ScalarExitValue represents a (loop-closed) phi, and as such can be inserted at the top of MiddleVPBB?

8756

Early-exit if loop has multiple exit blocks, at the outset?

8759

Early-exit instead, if ExitBB has more that one predecessor, at the outset?

8764

This should get rather than add, right?

We could have loop invariant incoming values which won't be modeled in VPlan, hence getOrAddVPValue.

Hmm, IncomingValue is initially an IR Instruction inside the loop feeding a loop-closed phi; if it is loop invariant (missed pre-LV LICM) then LV could LICM and move it to the pre-header (analogous to hoisting up a live-out IV) but instead VPlan keeps it in place wrapping it up with a uniform ReplicateRecipe?

8907

Exit values of inductions are disconnected from the loop body by precomputing them in preheader, so modeling them as VPInstructions in the middle block would require also modeling their precomputation in the preheader?

We should be able to add those to the pre-header, but I think it's best to do it one step at a time?

Sure, by all means.

10650

Populate the exit block of the epilog vector loop with scalar exit value recipes, only to discard them all here?

llvm/lib/Transforms/Vectorize/VPlan.cpp
335

Sounds right, but is this really needed - ScalarExitValue recipes seem to only hook-up loop-closed phis (at this time), w/o generating any instructions using Builder?

823

nit: seems more logical to generate code when called for the last part rather than the first, bailing out here if (Part != State.UF - 1), and using Part later rather than State.UF - 1?

828

Indicate that ExitValue usesScalars()?

Added usesScalars implementation.

It actually usesFirstOrLastLane only... perhaps something to think of in the future.

828

Perhaps worth emphasizing in the name - reduction epilogues using vectors probably deserve a separate "Exit Value" recipe.

Good point, not sure what the ideal name would be. Changed ScalarExitValue but I am not sure if that is much better. They model all exit values that do not need special handling.

Hmm, it seems ScalarExitValue merely models/wraps an IR loop-closed-ssa-phi taking care only of hooking it's operand. Perhaps deserves a "VPLiveOut" or "VPLoopClosedPhi" subclass of VPUser?

831

Placing ScalarExitValues inside middleBlock suggests they represent loop-closed phi's that will appear there, but actually the loop-closed phi's reside all along in Exit block, and use ScalarExitValue to retrieve its parent - middleBlock when rewiring its operand(?)

Test pr51366 below shows a ScalarExitValue inside 'exit' block which follows 'middleBlock'. Wonder if they are meant to be fused, provided 'exit' block has no other predecessor, or should ScalarExitValue introduce a loop-closed phi in middleBlock instead?

llvm/test/Transforms/LoopVectorize/X86/pr51366-sunk-instruction-used-outside-of-loop.ll
45

This GEP_LCSSA phi was hooked-up to TMP2 by a ScalarExitValue, right?

fhahn edited the summary of this revision. (Show Details)Apr 28 2022, 12:31 PM
fhahn updated this revision to Diff 426074.Apr 29 2022, 8:53 AM
fhahn marked 3 inline comments as done.

Address latest comments, thanks!

fhahn updated this revision to Diff 426076.Apr 29 2022, 8:59 AM
fhahn marked 3 inline comments as done.

Remove redundant loop.

fhahn marked 3 inline comments as done.Apr 29 2022, 9:06 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4107–4108

It is in the latest version of the patch. One wrinkle is that we need to update the ScalarExitValue instructions in the the plan for the epilogue loop. At the moment this is done by removing the exit values referring the LCSSA phi's for the main loop's exits and adding exit values for the newly created exit block.

That unfortunately requires exposing a bit more functionality to be able to do so.

8755

Added a comment, thanks!

8756

Added check for single predecessor, thanks!

8759

Added early exit, thanks!

8764

Yes that would be an option. It is likely only relevant for handwritten tests.

10650

The issue is that at this point, the exit values do not refer to the LCSSA phis created for the new exit block. I updated the code to re-add the exit values for the newly create exit block after executing the main plan. This allows removing fixLCSSAPHIs.

llvm/lib/Transforms/Vectorize/VPlan.cpp
335

We might need to insert extracts I think.

831

Placing ScalarExitValues inside middleBlock suggests they represent loop-closed phi's that will appear there, but actually the loop-closed phi's reside all along in Exit block, and use ScalarExitValue to retrieve its parent - middleBlock when rewiring its operand(?)

The recipe doesn't create new LCSSA phis, but only updates existing ones. It may need to insert extracts, which should be placed in the middle block.

Test pr51366 below shows a ScalarExitValue inside 'exit' block which follows 'middleBlock'. Wonder if they are meant to be fused, provided 'exit' block has no other predecessor, or should ScalarExitValue introduce a loop-closed phi in middleBlock instead?

I think the exit block can have multiple predecessors, in the test case it also has the latch of the scalar loop as predecessor. We could also generate an LCSSA phi in the middle block, but it doesn't seem needed currently.

llvm/test/Transforms/LoopVectorize/X86/pr51366-sunk-instruction-used-outside-of-loop.ll
45

Yep exactly.

Ayal added inline comments.May 2 2022, 12:15 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4107–4108

Perhaps better to progress gradually - first introduce loop-closed phi recipes inside Middle block (admittedly affecting many tests?), and model VPlan live-out VPUsers + update VPlan's fixLCSSAPHIs() afterwards?

8755

ScalarExitValue represents a (loop-closed) phi, and as such can be inserted at the top of MiddleVPBB?

Ah, original comment suggested that insert point could alternatively be set to MiddleVPBB->begin() instead of MiddleVPBB->getFirstNonPhi() for ScalarExitValue. But that is incorrect - ScalarExitValue currently represents the rewiring of a loop-value to a loop-closed phi residing in Exit block, rather than a loop-closed phi in Middle block, along with an implicit potential extract - which must appear in MiddleBlock following all its phi's.

A recipe is designed to take care of generating an instruction (one or more) and hooking it up to its operands, rather than taking care of its users.

The term "live-out" may be ambiguous - referring to values defined inside the loop body and used outside of it, and/or values defined inside VPlan's scope and used outside of it. By definition, VPlan live-outs should be represented using a derivative of VPUser that is not a recipe - they must have underlying Instruction/User, which need to be rewired, but belong to no VPBB, potentially held by VPlan itself; analogous to VPlan live-ins represented using VPValues that are not part of any recipe's VPDef, and held in VPlan's VPExternalDefs.

8764

Original comment just notes that this should get rather than add, even if incoming value is loop invariant.

llvm/lib/Transforms/Vectorize/VPlan.cpp
335

Ah, right! Better to dispel this magic rather than place a recipe in a given VPBB for no apparent reason.

823

sounds right?

831

Placing ScalarExitValues inside middleBlock suggests they represent loop-closed phi's that will appear there, but actually the loop-closed phi's reside all along in Exit block, and use ScalarExitValue to retrieve its parent - middleBlock when rewiring its operand(?)

The recipe doesn't create new LCSSA phis, but only updates existing ones. It may need to insert extracts, which should be placed in the middle block.

Let's try to use recipes, represent VPlan live-outs and model loop-closed phi's more explicitly and clearly.

Test pr51366 below shows a ScalarExitValue inside 'exit' block which follows 'middleBlock'. Wonder if they are meant to be fused, provided 'exit' block has no other predecessor, or should ScalarExitValue introduce a loop-closed phi in middleBlock instead?

I think the exit block can have multiple predecessors, in the test case it also has the latch of the scalar loop as predecessor. We could also generate an LCSSA phi in the middle block, but it doesn't seem needed currently.

Generating an LCSSA phi in the middle block, using a dedicated recipe/VPInstruction, seems most reasonable - it would in turn feed the original loop-closed phi of the Exit block (along with an extract if needed), represented as a VPlan live-out VPUser?

fhahn updated this revision to Diff 427045.May 4 2022, 9:48 AM
fhahn marked 4 inline comments as done.

Update the patch to use a dedicated user class (VPExitValue) instead of piggybacking on VPInstruction.

fhahn added a comment.May 10 2022, 6:53 AM

ping :)

Comments should be addressed in the most recent update.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4107–4108

I think we should try to fix the exit-value case first, as this can currently cause crashes and mis-compiles. I could undo the changes here and keep using fixLCSSAPHIs for the epilogue case. What do you think?

8755

Update to use a dedicated user for exit values.

8764

Ah right! I think we still may need to add a value, e.g. in case if the exit value is only used in the exit phi.

llvm/lib/Transforms/Vectorize/VPlan.cpp
831

Updated the patch to use a separate VPUser for instead a recipe.

fhahn edited the summary of this revision. (Show Details)May 15 2022, 9:15 AM
Ayal added a comment.May 16 2022, 2:46 PM

Summary deserves updating.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8756

Comment needs updating: exit values are now represented as VPUsers rather than recipes, they belong to VPlan - 'residing' outside any of VPlan's VPBasicBlock, rather than be added to the beginning of exit/middle block.
No need to set insertion point.

llvm/lib/Transforms/Vectorize/VPlan.cpp
335

Better to set the insertion point to middle block when calling fixPhi's, rather than here, noting that the latter may need to generate extracts there?

1052

Set the insertion point of State's builder here?

1101

Should this check for duplicates, as in getOrAdd?

llvm/lib/Transforms/Vectorize/VPlan.h
678

nit: "recipe"

2526

Can clear exit values regardless if Entry or not?

Ayal added inline comments.May 16 2022, 2:46 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
10649

change independent of this patch?

10653–10654

change independent of this patch?

fhahn updated this revision to Diff 430155.May 17 2022, 12:13 PM
fhahn marked 8 inline comments as done.

Address comments, greatly simplify patch. The new version adds VPExitValues for all values used by LCSSA phis. Non-vplan based helper (fixupIVUsers, fixFirstOrderRecurrence, fixReduction) remove VPExitValues for LCSSA phis they handle directly, before fixing the remaining VPExitValues.

Follow-up patches will gradually replace those helpers with dedicated users in VPlan.

fhahn added inline comments.May 17 2022, 12:15 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8756

This code is now completely gone.

10649

Done in 5b00d13c0071.

10653–10654

Done in 5b00d13c0071.

llvm/lib/Transforms/Vectorize/VPlan.cpp
335

Should be adjusted in the current version, thanks!

1052

Done in the latest version (at the new place this is called)

1101

I think this should never be called multiple times with the same PN. Added an assertion.

llvm/lib/Transforms/Vectorize/VPlan.h
678

Fixed, thanks!

2526

Fixed, thanks!

Ayal added inline comments.May 17 2022, 1:31 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
10653–10654

While we're here, first set Header then set its name?

10665

How do the new exit values created by addUsersInExitBlock() below differ from the existing exit values of BestEpiPlan they replace? Both wrap the same LCSSA phis and presumably use the same VPValue from within BestEpiPlan. When called to fix their phi, they should update distinct incoming values corresponding to distinct middle blocks, but those are independent of the exit values themselves - that's why Plan is provided to fixPhi()?

10674

This processLoop() method is becoming 388 LOC long... any change to it should consider refactoring?

llvm/lib/Transforms/Vectorize/VPlan.cpp
1081

Perhaps call them VPLiveOut/VPLiveOutIR/VPLiveOutUser/VPLiveOutIRUser rather than VPExitValue, given how they are printed, and to complement live-in IR Values which are represented via VPDef-less VPValues supporting getLiveInIRValue()?

assert/verify/note somewhere that VPExitValue is assumed to have a single operand?

llvm/lib/Transforms/Vectorize/VPlan.h
685

const?
(Currently used for printing only.)

2630

Change related to this patch?
Can a test demonstrate its need?

fhahn updated this revision to Diff 430274.May 18 2022, 1:41 AM
fhahn marked 8 inline comments as done.

Address latest comments, thanks!

fhahn edited the summary of this revision. (Show Details)May 18 2022, 1:41 AM
fhahn retitled this revision from [VPlan] Model first exit values in VPlan. to [VPlan] Model first exit values using VPLiveOut.
fhahn retitled this revision from [VPlan] Model first exit values using VPLiveOut to [VPlan] Model first exit values using VPLiveOut..
Ayal added a comment.May 19 2022, 11:20 PM

Looks good to me! Adding minor suggestions.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
591

Worth retaining this explanation when documenting LiveOut/fixPhi()?

3394

Suffice to remove LiveOut only when actually fixing the incoming, i.e., under the if?

3719

I.e., there’s nothing to fix from vector loop, phi should have incoming from scalar loop only?

3720

Worth reversing the condition to handle the simpler case first?

Or have addUsersInExitBlock() bail out early if requiresScalarEpilogue()?

3722

Set to first non phi instead of begin, as we prepare for non phi extracts, although middle block is free of (LCSSA) phi’s atm?

May as well pass MiddleBlock to fixPhi, having directed Builder to it, instead of having each fixPhi derive it from Plan? (Can also have fixPhi derive MiddleBlock from Builder…)

3877

Single else placed earlier suffices, no need for same else here as well?

3935

In general LiveOut exits from VPlan to IR rather than from Loop. Distinction more important for outerloop vectorization.

4063

A bit confusing when to else cleanup LiveOuts; perhaps do so directly rather than as an else, or when creating LiveOuts?

8711

Can fold both early exits into one.

Also early exit if requiresScalarEpilogue()?

9159

ditto regarding retention of LoopExitValue vs. VPlanLiveOut.

llvm/lib/Transforms/Vectorize/VPlan.cpp
651

nits: set ExitingVPValue to operand(0), used twice.
Derive and set MiddleVPBock from loop/outermost region enclosing ExitingVPValue, rather than from Plan? Or OTOH have it passed in, as it’s the same for all phis to fix.

1102

assert count is zero?

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
212

nit: empty line before final return true?

fhahn updated this revision to Diff 430905.May 20 2022, 1:43 AM
fhahn marked 13 inline comments as done.

Address latest comments, thanks!

The latest version also removes a workaround to detect users outside the loop using IR def-use chains (HasUsersOutsideLoop).

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
591

I moved the comment to VPLiveOut::fixPhi.

3394

Yes that should be sufficient, updated!

3719

Reworded the comment, thanks!

3720

Worth reversing the condition to handle the simpler case first?

Done!

Or have addUsersInExitBlock() bail out early if requiresScalarEpilogue()?

I think that would have the drawback that we might miss uses outside the vector loop in VPlan analyses. The latest version of the patch removed a workaround that required us checking the IR def-use chains to detect outside users.

3722

Set to first non phi instead of begin, as we prepare for non phi extracts, although middle block is free of (LCSSA) phi’s atm?

Done thanks!

May as well pass MiddleBlock to fixPhi, having directed Builder to it, instead of having each fixPhi derive it from Plan? (Can also have fixPhi derive MiddleBlock from Builder…)

I updated fixPhi to use the current block from the builder instead, seemed simpler.

3877

I think I might be missing something. Are you referring to removing all live outs earlier in fixVectorizedLoop via Plan.clearLiveOuts? This should only cover the case where we require scalar epilogues, whereas here we only deal with the other case.

3935

I'll undo the change, this was just an accidental replacement.

4063

I'm not entirely sure what you are suggesting here. The reason this is done here to only remove the live-out of the current reduction. Doing so when creating LiveOuts is more complicated (as in earlier versions of the patch).

8711

Can fold both early exits into one.

Done, thanks!

Also early exit if requiresScalarEpilogue()?

I think we should still create live-outs if requiresScalarEpilogue, otherwise some recipes may be considered dead if they only have uses outside the loop. The latest version of the patch uses that property to remove a workaround in VPlanTransforms that used IR def use chains to detect users outside the loop.

9159

Changed back thanks!

10653–10654
10665

I think the comment was for an earlier version, the code is gone now. Depends on D125810

10674

I think this comment was for an earlier version of the patch, the changes are gone now

llvm/lib/Transforms/Vectorize/VPlan.cpp
651

nits: set ExitingVPValue to operand(0), used twice.

Done, thanks!

Derive and set MiddleVPBock from loop/outermost region enclosing ExitingVPValue, rather than from Plan? Or OTOH have it passed in, as it’s the same for all phis to fix.

Updated to get the block from the builder directly.

828

Do you mean in addition to VPScalarExitValue? I've not added a new subclass in this patch as perhaps it would be good add it once we have multiple sub-classes?

1081

Renamed to VPLiveOut &LiveOut.

I added a check to the VPlan verifier.

1102

adjusted, thanks!

llvm/lib/Transforms/Vectorize/VPlan.h
685

Adjusted, thanks!

2630

I forgot to remove this change, it's not needed any longer. Removed, thanks!

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
212

Adjusted, thanks!

Ayal accepted this revision.May 20 2022, 5:30 AM

Looks good to me, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3714

OK, this is where LCSSA phis were fixed originally, can clear LiveOuts here if requiresScalarEpilogue().

3877

Yes; we’re also checking if requiresScalarEpilogue() here, as done earlier and later when dealing with various reductions/inductions/recurrences

4063

Was only referring to avoiding creating any LiveOut if requiresScalarEpilogue().

8711

Also early exit if requiresScalarEpilogue()?

I think we should still create live-outs if requiresScalarEpilogue, otherwise some recipes may be considered dead if they only have uses outside the loop. The latest version of the patch uses that property to remove a workaround in VPlanTransforms that used IR def use chains to detect users outside the loop.

This perhaps deserves a comment somewhere, explaining that VPLiveOuts are used to represent loop live out values that originally fed LCSSA phis in Exit Block, where these phis may or may not require fixing - if their edge is removed due to required scalar epilogue - in which case they are still live out but feed only preheader of scalar epilogue?

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
386

Nice cleanup!

This revision is now accepted and ready to land.May 20 2022, 5:30 AM
This revision was landed with ongoing or failed builds.May 21 2022, 8:15 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked 2 inline comments as done.
fhahn added inline comments.May 21 2022, 9:44 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3714

Sounds good, that's what the latest version of the patch is doing.

3877

I think for now it is easiest to keep the check and remove it once we migrate exit value handling for those.

8711

I'll adjust the comment here.

Ayal added inline comments.May 21 2022, 12:49 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8711

I'll adjust the comment here.

Thanks!

Perhaps instead of deleting LiveOuts whose phis are fixed or need not be fixed due to required scalar epilog, such LiveOuts should be marked as "fixed" - so that queries of outside users still hold while preventing their future fixes(?). This may become clearer when external users of inductions/reductions/recurrences are introduced.

Hi @fhahn. This patch looks be the cause of the failure reported by https://lab.llvm.org/buildbot/#/builders/176/builds/1723. As well as on AArch64 I also see the failure on X86_64 when doing the following cross compile:

./bin/clang++ -target aarch64-linux-gnu --sysroot=/opt/aarch64-sysroot --gcc-toolchain=/opt/aarch64-cross-compiler -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Transforms/Scalar -I/home/pmw/projects/upstream-llvm/llvm-project/llvm/lib/Transforms/Scalar -Iinclude -I/home/pmw/projects/upstream-llvm/llvm-project/llvm/include -mcpu=a64fx -msve-vector-bits=512 -mllvm -treat-scalable-fixed-error-as-warning=false -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG    -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT lib/Transforms/Scalar/CMakeFiles/LLVMScalarOpts.dir/SimpleLoopUnswitch.cpp.o -MF lib/Transforms/Scalar/CMakeFiles/LLVMScalarOpts.dir/SimpleLoopUnswitch.cpp.o.d -o SimpleLoopUnswitch.cpp.o -c /home/pmw/projects/upstream-llvm/llvm-project/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

The failure disappears after locally reverting this patch.

Hi @fhahn. This patch looks be the cause of the failure reported by https://lab.llvm.org/buildbot/#/builders/176/builds/1723. As well as on AArch64 I also see the failure on X86_64 when doing the following cross compile:

./bin/clang++ -target aarch64-linux-gnu --sysroot=/opt/aarch64-sysroot --gcc-toolchain=/opt/aarch64-cross-compiler -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Transforms/Scalar -I/home/pmw/projects/upstream-llvm/llvm-project/llvm/lib/Transforms/Scalar -Iinclude -I/home/pmw/projects/upstream-llvm/llvm-project/llvm/include -mcpu=a64fx -msve-vector-bits=512 -mllvm -treat-scalable-fixed-error-as-warning=false -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG    -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT lib/Transforms/Scalar/CMakeFiles/LLVMScalarOpts.dir/SimpleLoopUnswitch.cpp.o -MF lib/Transforms/Scalar/CMakeFiles/LLVMScalarOpts.dir/SimpleLoopUnswitch.cpp.o.d -o SimpleLoopUnswitch.cpp.o -c /home/pmw/projects/upstream-llvm/llvm-project/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

The failure disappears after locally reverting this patch.

Thanks for the heads up! I pushed 97590baead08 which should fixe the issue. I am keeping an eye on the bot, it looks like it made it past building SimpleLoopUnswitch.cpp already.

This brings unstable behavior.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3724

This loop is affected by DenseMap.

fhahn marked an inline comment as done.May 25 2022, 1:33 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3724

Thanks, should be fixed by 1ba42dd04b8d