This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Enhance the simplification of `copyto` from `implicit-def`.
ClosedPublic

Authored by hliao on May 23 2019, 1:45 PM.

Details

Summary
  • The current implementation simplifies the case where the source of copyto is implicit-defed. However, it only works when that implicit-def is single-used since it detects that from implicit-def and cannot determine which destination vreg should be used if there are multiple uses.
  • This patch changes that detection when copyto is being emitted. If that copyto's source is defined from implicit-def, it simplifies it. Hence, it works even that implicit-def is multi-used.
  • Except it simplifies the internal IR, it won't improve the quality of code generation. However, it helps to detect 'implicit-def` in a straight-forward manner in some passes, such as si-i1-copies. A test case is added.

Diff Detail

Repository
rL LLVM

Event Timeline

hliao created this revision.May 23 2019, 1:45 PM
hliao updated this revision to Diff 201125.May 23 2019, 9:53 PM

revise the case, there's no improvement or regression in the existing test.

nhaehnle requested changes to this revision.May 24 2019, 12:48 AM

I don't think this is correct. If an IMPLICIT_DEF has multiple uses, then the correctness of the program may depend on all uses seeing the same value, even if that value is undefined.

The description also doesn't line up with the test description (which talks about making sure that something doesn't crash).

This revision now requires changes to proceed.May 24 2019, 12:48 AM
hliao added a comment.EditedMay 24 2019, 5:34 AM

I don't think this is correct. If an IMPLICIT_DEF has multiple uses, then the correctness of the program may depend on all uses seeing the same value, even if that value is undefined.

The description also doesn't line up with the test description (which talks about making sure that something doesn't crash).

IMPLICIT_DEF is multi-instantiated in lowering, i.e. each use of it has a dedicated definition of IMPLICIT_DEF (refer to getVR and getDstOfOnlyCopyToRegUse). The only slight simplification is for the sequence of COPY_TO followed by IMPLICIT_DEF on virtual register dest. In that case, that COPY_TO could be eliminated. IMPLICIT_DEF is just undef, the implementation has the right to decide how to do it efficiently.

arsenm added a subscriber: arsenm.May 24 2019, 5:38 AM

I don't think this is correct. If an IMPLICIT_DEF has multiple uses, then the correctness of the program may depend on all uses seeing the same value, even if that value is undefined.

I don't think this is a guaranteed property. This is the MIR equivalent of the poison/freeze question, but today implicit_def doesn't give you this

arsenm added inline comments.May 24 2019, 5:39 AM
llvm/test/CodeGen/AMDGPU/i1-copy-phi.ll
41–42 ↗(On Diff #201125)

This should check something. I also suspect it can be reduced further

hliao updated this revision to Diff 201225.May 24 2019, 6:28 AM

reduce test further

hliao marked an inline comment as done.May 24 2019, 6:29 AM

test is reduced further

hliao updated this revision to Diff 201226.May 24 2019, 6:47 AM

reduce even further

hliao marked an inline comment as done.May 24 2019, 6:48 AM
hliao added inline comments.
llvm/test/CodeGen/AMDGPU/i1-copy-phi.ll
41–42 ↗(On Diff #201125)

test is reduced

hliao edited the summary of this revision. (Show Details)May 24 2019, 6:50 AM
nhaehnle accepted this revision.May 27 2019, 6:51 AM

I don't think this is correct. If an IMPLICIT_DEF has multiple uses, then the correctness of the program may depend on all uses seeing the same value, even if that value is undefined.

I don't think this is a guaranteed property. This is the MIR equivalent of the poison/freeze question, but today implicit_def doesn't give you this

Hmm alright. Then the change itself seems okay.

This revision is now accepted and ready to land.May 27 2019, 6:51 AM
This revision was automatically updated to reflect the committed changes.