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.
Differential D77795
[GlobalISel] translate freeze to new generic G_FREEZE gargaroff on Apr 9 2020, 5:57 AM. Authored by
Details As a follow up to https://reviews.llvm.org/D29014, add translation Introduce a new generic instruction G_FREEZE and translate freeze to it.
Diff Detail
Event TimelineComment Actions LGTM in terms of semantic correctness. Comment Actions Probably best to wait a bit before committing in case any GlobalIsel contributors have any thoughts. Comment Actions This doesn't match how the DAG handles it. This should use a dedicated G_FREEZE like the DAG, selected later Comment Actions I think it matches with what the current SelDAG does. Comment Actions 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)); } Comment Actions 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 Comment Actions 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 Comment Actions 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. Comment Actions 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. Comment Actions remove unnecessary llvm:: add buildFreeze to MachineIRBuilder add struct freeze test case use argument instead of constant in test
Comment Actions 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 |
Don't need llvm::