This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Handle debug instrs in ARM Low Overhead Loop pass
ClosedPublic

Authored by vhscampos on Mar 22 2021, 7:01 AM.

Details

Summary

In function ConvertVPTBlocks(), it is assumed that every instruction
within a vector-predicated block is predicated. This is false for debug
instructions, used by LLVM.

Because of this, an assertion failure is reached when an input contains
debug instructions inside VPT blocks. In non-assert builds, an out of
bounds memory access took place.

The present patch properly covers the case of debug instructions.

Diff Detail

Event Timeline

vhscampos created this revision.Mar 22 2021, 7:01 AM
vhscampos requested review of this revision.Mar 22 2021, 7:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2021, 7:01 AM
dmgreen added inline comments.
llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
1466–1471

This looks like it should be an assert.

1516–1519

I think this might have to handle debug better. Can we try and make sure that DivergentNext is not a debug instruction.

vhscampos updated this revision to Diff 332327.Mar 22 2021, 9:24 AM

Add assertion in RemovePredicate lambda.
SKip debug instructions inside VPT blocks with predicate divergence.

vhscampos marked 2 inline comments as done.Mar 22 2021, 9:24 AM
dmgreen added inline comments.Mar 22 2021, 1:32 PM
llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
1520

I'm not sure if DivergentNext can be end(), can it? I think the block will always contain a terminator.
(If it can be end(), we would still need to remove the predicates and replace the instruction)

vhscampos updated this revision to Diff 332608.Mar 23 2021, 4:14 AM

Remove predicate of instructions after the divergent one even if there's no non-debug instruction after it.

dmgreen accepted this revision.Mar 23 2021, 4:19 AM

Thanks for the fix. LGTM.

llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
1466–1471

It doesn't need both the assert and the unreachable - you can remove the if.

This revision is now accepted and ready to land.Mar 23 2021, 4:19 AM
vhscampos updated this revision to Diff 332622.Mar 23 2021, 4:45 AM

Remove if statement inside RemovePredicate as it's already covered by an assertion.

vhscampos marked 2 inline comments as done.Mar 23 2021, 4:46 AM
This revision was landed with ongoing or failed builds.Mar 23 2021, 4:49 AM
This revision was automatically updated to reflect the committed changes.