This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][AMDGPU] add legalization for G_FREEZE
ClosedPublic

Authored by gargaroff on Apr 14 2020, 2:41 AM.

Details

Summary

Copy the legalization rules from SelectionDAG:
-widenScalar using anyext
-narrowScalar using intermediate merges
-scalarize/fewerElements using unmerge
-moreElements using G_IMPLICIT_DEF and insert

Add G_FREEZE legalization actions to AMDGPULegalizerInfo.
Use the same legalization actions as G_IMPLICIT_DEF.

Depends on D77795.

Diff Detail

Event Timeline

gargaroff created this revision.Apr 14 2020, 2:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2020, 2:41 AM
gargaroff updated this revision to Diff 257250.Apr 14 2020, 2:45 AM

remove misleading comment

Our backend doesn't support vectors, so I'm really not too familiar with the legalization of them. I tried to implement vector legalization to the best of my understanding, but if there are any problems with those, I'd be very grateful if you could point me in the right direction.

arsenm added inline comments.Apr 15 2020, 5:20 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
843–846

I've been working on eliminating these somewhat awkward functions that end up producing G_INSERT/G_EXTRACT in odd breakdowns instead of creating intermediate merges to the requested type. fewerElementsVectorBasic should be pretty close to what this should do? (Although maybe pad with undef is the wrong choice here)

856–857

Should have a dedicated buildFreeze

1736

Is this what the DAG does? I would be slightly surprised that undef high bits would be OK

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp
2970

If you want some tests that are less inconvenient than unit tests, for AMDGPU this should have the same legalization action as G_IMPLICIT_DEF (and probably true for all targets)

gargaroff marked an inline comment as done.Apr 15 2020, 7:23 AM
gargaroff added inline comments.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1736

I'm not familiar with SelectionDAG, so I might have misunderstood the extension used.

This is the code in the DAG:

SDValue DAGTypeLegalizer::PromoteIntRes_FREEZE(SDNode *N) {
  SDValue V = GetPromotedInteger(N->getOperand(0));
  return DAG.getNode(ISD::FREEZE, SDLoc(N),
                     V.getValueType(), V);
}

In the documentation of GetPromotedInteger it says that the upper bits contain rubbish, which suggested to me that this corresponds to G_ANYEXT

aqjune added inline comments.Apr 15 2020, 7:09 PM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1736

In case of SelDag, I think PromoteIntRes_FREEZE is correct: it is creating FREEZE instruction, so even if GetPromotedInteger returned undef bits, the result is frozen, so the high bits are frozen bits.

gargaroff updated this revision to Diff 258050.Apr 16 2020, 7:28 AM
gargaroff marked 7 inline comments as done.

add legalization action for G_FREEZE to AMDGPULegalizerInfo

rewrite narrowScalar for G_FREEZE to use intermediate merges instead of extract/insert

extract G_FREEZE narrowing to narrowScalarFreeze

gargaroff added inline comments.Apr 16 2020, 7:29 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
843–846

It was pretty close to what was needed indeed. I just had to change some parameters to some function calls, but otherwise it's mostly a copy of fewerElementsVectorBasic.

I didn't bother with refactoring both to reuse most of the unchanged code yet to get a first feedback on the narrowing action. Also I need to check how well the code can be shared in the first place. Btw, if you have a suggestion on how such a shared function could be named, I'm open for suggestions because I'm struggling to come up with a meaningful name.

Also, I decided to use undef padding here, since @aqjune said that undef bits are fine, because they get frozen anyway.

llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp
2970

I went ahead and added the same legalization actions for G_FREEZE as G_IMPLICIT_DEF and have basically copied all tests from legalizeImplicitDef to legalizeFreeze and modified them to use G_FREEZE.

gargaroff retitled this revision from [GlobalISel] add legalization for G_FREEZE to [GlobalISel][AMDGPU] add legalization for G_FREEZE.Apr 16 2020, 7:46 AM
gargaroff edited the summary of this revision. (Show Details)
arsenm added inline comments.Apr 16 2020, 8:07 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4152

Maybe reduceOperationWidth? We have a reduceLoadStoreWidth shared between fewerElementsVector and narrowScalar

gargaroff updated this revision to Diff 258258.Apr 17 2020, 1:47 AM

refactor narrowScalarFreeze, fewerElementsVectorBasic to general reduceOperationWidth

gargaroff marked an inline comment as done.Apr 17 2020, 1:48 AM
arsenm accepted this revision.Apr 17 2020, 6:54 AM
arsenm added inline comments.
llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-freeze.mir
93

_48->_s48

This revision is now accepted and ready to land.Apr 17 2020, 6:54 AM
gargaroff marked an inline comment as done.Apr 17 2020, 7:45 AM
This revision was automatically updated to reflect the committed changes.