This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] translate freeze to new generic G_FREEZE
ClosedPublic

Authored by gargaroff on Apr 9 2020, 5:57 AM.

Details

Summary

As a follow up to https://reviews.llvm.org/D29014, add translation
support for freeze.

Introduce a new generic instruction G_FREEZE and translate freeze to it.

Diff Detail

Event Timeline

gargaroff created this revision.Apr 9 2020, 5:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2020, 5:57 AM
gargaroff updated this revision to Diff 256269.Apr 9 2020, 6:05 AM

remove unrelated style changes from diff

lebedev.ri added a subscriber: lebedev.ri.

This looks consistent with what we currently do in SelDAG, so LG in principle.

aqjune accepted this revision.Apr 9 2020, 6:17 AM

LGTM in terms of semantic correctness.
Would this need another reviewer's acceptance to assure it's okay?

This revision is now accepted and ready to land.Apr 9 2020, 6:17 AM
fhahn added a subscriber: fhahn.

Probably best to wait a bit before committing in case any GlobalIsel contributors have any thoughts.

Harbormaster completed remote builds in B52502: Diff 256266.
arsenm requested changes to this revision.Apr 9 2020, 7:34 AM

This doesn't match how the DAG handles it. This should use a dedicated G_FREEZE like the DAG, selected later

This revision now requires changes to proceed.Apr 9 2020, 7:34 AM
aqjune added a comment.EditedApr 9 2020, 7:40 AM

I think it matches with what the current SelDAG does.
In the previous FREEZE patch the MachineInstr part didn't reach consensus, but SelDag part reached, so only the part was merged into master first. Currently it is lowered into MachineInstr's COPY.
I promised to make the MachineIR part in the future, but it didn't seem to be very high priority right now, so not made yet.

aqjune added a comment.Apr 9 2020, 7:41 AM

The corresponding part in SelectionDAGISel looks like this:

void SelectionDAGISel::Select_FREEZE(SDNode *N) {
  // TODO: We don't have FREEZE pseudo-instruction in MachineInstr-level now.
  // If FREEZE instruction is added later, the code below must be changed as
  // well.
  CurDAG->SelectNodeTo(N, TargetOpcode::COPY, N->getValueType(0),
                       N->getOperand(0));
}
arsenm added a comment.Apr 9 2020, 9:13 AM

The corresponding part in SelectionDAGISel looks like this:

void SelectionDAGISel::Select_FREEZE(SDNode *N) {
  // TODO: We don't have FREEZE pseudo-instruction in MachineInstr-level now.
  // If FREEZE instruction is added later, the code below must be changed as
  // well.
  CurDAG->SelectNodeTo(N, TargetOpcode::COPY, N->getValueType(0),
                       N->getOperand(0));
}

This isn't equivalent. This is the selection of the intermediate ISD::FREEZE, not directly emitting a copy from the IR. The equivalent is here: https://github.com/llvm/llvm-project/blob/015dee1ac8988ed5184ef649c72ea0692f2633b3/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L10549

arsenm added a comment.Apr 9 2020, 9:15 AM

The copy won't be legalized, so you're going to have issues emitting a copy for any non-legal source. We need a legalizable G_FREEZE

The copy won't be legalized, so you're going to have issues emitting a copy for any non-legal source. We need a legalizable G_FREEZE

How should legalization rules look like? Since the value has to be stable I'm guessing zero extend for widen and unmerge/merge/extract/insert for narrow?

If we don't need to add selection support to any backend I could do that tomorrow.

My only concern is that the maintenance required to eventually introduce a new FREEZE MachineInstr which G_FREEZE should be selected to is quite a bit higher, since all GlobalISel backends that support G_FREEZE must be updated to properly select to FREEZE, instead of the COPY, while only having to change the IRTranslator is a lot easier.

But if legalization is a problem, then I guess there's nothing else that we can do.

The copy won't be legalized, so you're going to have issues emitting a copy for any non-legal source. We need a legalizable G_FREEZE

How should legalization rules look like? Since the value has to be stable I'm guessing zero extend for widen and unmerge/merge/extract/insert for narrow?

The same as whatever SelectionDAG does? It has the full set of legalize handling

The copy won't be legalized, so you're going to have issues emitting a copy for any non-legal source. We need a legalizable G_FREEZE

How should legalization rules look like? Since the value has to be stable I'm guessing zero extend for widen and unmerge/merge/extract/insert for narrow?

The same as whatever SelectionDAG does? It has the full set of legalize handling

Oh, right. Seems like I was totally getting ahead of myself 😅

Anyway. I'll try to come up with something tomorrow or over the weekend. Maybe just for scalars as a first step. If vectors turn out to not be too complex either, I'll try to add them as well.

gargaroff updated this revision to Diff 257247.Apr 14 2020, 2:38 AM

introduce new generic instruction G_FREEZE

gargaroff retitled this revision from [GlobalISel] translate freeze to COPY to [GlobalISel] translate freeze to new generic G_FREEZE.Apr 14 2020, 2:39 AM
gargaroff edited the summary of this revision. (Show Details)
gargaroff updated this revision to Diff 257271.Apr 14 2020, 3:53 AM

add missing check lines to legalizer-info-validation.mir

arsenm added inline comments.Apr 14 2020, 6:52 AM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2107–2108

Don't need llvm::

2111

Should add a dedicated buildFreeze

llvm/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
2372

Can you add a struct freeze case? Also maybe use an argument instead of a constant?

gargaroff updated this revision to Diff 257696.Apr 15 2020, 6:21 AM

remove unnecessary llvm::

add buildFreeze to MachineIRBuilder

add struct freeze test case

use argument instead of constant in test

gargaroff marked 3 inline comments as done.Apr 15 2020, 6:21 AM
arsenm accepted this revision.Apr 15 2020, 6:38 AM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
892 ↗(On Diff #257696)

This can just go in the header

This revision is now accepted and ready to land.Apr 15 2020, 6:38 AM
gargaroff closed this revision.Apr 15 2020, 7:49 AM
gargaroff marked an inline comment as done.

Seems like the pre-merge build bot is broken, since it's just showing me an HTML error instead of the build log. Everything should pass, so I went ahead and landed the change: 443c244cff6ac43735654b2d2c74ca06e7bab102