Page MenuHomePhabricator

[LSR] RFC: Fix crash in Phi node with EHPad block
Needs ReviewPublic

Authored by lorenz on Mar 10 2021, 4:49 PM.

Details

Summary

This fixes a crash I observed in issue #48708 where the LSR
pass tries to insert an instruction in a basic block with only a
catchswitch statement in there. This happens because the Phi node
being evaluated has two incoming values which are reference-equal
but different basic blocks.

If the basic block associated with the incoming value of the operand
being evaluated has an EHPad terminator LSR drops that in the check
just above the one I added. But if that incoming value is reference-
equal to another incoming value, RewriteForPhi ends up rewriting
both values, one of which can have an EHPad terminator and thus
hits the "Insertion point must be a normal instruction" assert in
AdjustInsertPositionForExpand.

This fix makes CollectLoopInvariantFixupsAndFormulae also ignore
cases where a reference-equal value has a basic block with an
EHPad, same as it already does in case the primary value has one.

Diff Detail

Event Timeline

lorenz created this revision.Mar 10 2021, 4:49 PM
lorenz requested review of this revision.Mar 10 2021, 4:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2021, 4:49 PM

Reviewers: Please note that I'm completely unfamiliar with the LLVM codebase. This might be complete nonsense. But it does fix my issue.

lorenz edited the summary of this revision. (Show Details)Mar 13 2021, 2:30 PM
lorenz added a reviewer: qcolombet.

Hi,

Could you add a test case exposing the problem?

Ideally something that just needs to run LSR: opt -loop-reduce problematic_ir.ll -S -o -

Cheers,
-Quentin

@qcolombet Have a look at the associated bug at https://bugs.llvm.org/show_bug.cgi?id=48708, there's a minimized IR reproducer in there.

@qcolombet Have a look at the associated bug at https://bugs.llvm.org/show_bug.cgi?id=48708, there's a minimized IR reproducer in there.

Great! Could you add that to this patch?

lorenz updated this revision to Diff 333171.Mar 24 2021, 4:20 PM

Add reproducer as test case

qcolombet added inline comments.Mar 31 2021, 10:02 AM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
3452

Period at the end of comments.

3457

Instead of iterating through all the incoming values, can we use Use::getOperandNo to only check the relevant one?

llvm/test/Transforms/LoopStrengthReduce/X86/issue48708.ll
1

Could you add check lines to make sure we do something sensible?
No crashing doesn't mean we are correct :).

2

Could you add a comment on what this test is checking and what we should/shouldn't observe here?
In particular putting the PR id here is a good idea.

3

Could you change the name of the test case to something more descriptive?

11

Could you get rid of the implicit variables (%[0-9]+)?
You can use opt -instnamer -S for that.