This is motivated by an pathological compile time issue during unmerge combining.
We should be able to use the AVF to do simplification. However AMDGPU has a lot of codegen changes which I'm not sure how to evaluate.
Paths
| Differential D109748
[GlobalISel][Legalizer] Use ArtifactValueFinder first for unmerge combines before trying others. ClosedPublic Authored by aemerson on Sep 14 2021, 2:02 AM.
Details Summary This is motivated by an pathological compile time issue during unmerge combining. We should be able to use the AVF to do simplification. However AMDGPU has a lot of codegen changes which I'm not sure how to evaluate.
Diff Detail
Event TimelineHerald added subscribers: kerbowa, tpr, rovka and 2 others. · View Herald TranscriptSep 14 2021, 2:02 AM Comment Actions The only .ll test in AMDGPU seems to show a code improvement despite most MIR tests showing larger code. I can't make heads or tails of how AMDGPU is supposed to work so I'd appreciate some feedback on if this is ok. Comment Actions The MIR test changes are all clearly worse, but the IR tests are small improvements. The MIR tests all look like unfortunate legalization ordering changes that just result in more artifact churn (some of which should result in more instructions in the end). This is probably from no longer considering the legalization action of the proposed new unmerge. I'm not really sure what to do here. I'm still not 100% clear on how artifact legalization rules should be used, and whether the artifact combiner should be fully responsible for handling them Comment Actions
Can these regressions be cleaned up by a later (postlegalize) combine for AMDGPU? Comment Actions
In principle you can always go backwards. There are already a lot of intermediate steps for these artifacts already, and this would add yet another set on top of them. The changes I think are ugliest look like legalize-smin.mir. This is a trivial merge of legal register types, but now this turns into a mess of bitshifting and bitmasking. I think there would be improvements by changing the legalization order, or somehow considering the specific legalization of newly created artifacts Comment Actions
At least some of those changes seem to actually be due to different dead code being left behind by the legalizer. I've put D109858 to allow us to clean it up for tests. Comment Actions Rebase after the DCE change landed. Now the diffs show that code is no worse, and in some cases better than before. This revision is now accepted and ready to land.Sep 20 2021, 6:25 PM This revision was landed with ongoing or failed builds.Sep 21 2021, 12:02 AM Closed by commit rGcc65e08fe7e1: [GlobalISel][Legalizer] Use ArtifactValueFinder first for unmerge combines… (authored by aemerson). · Explain Why This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 373752 llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
llvm/test/CodeGen/AArch64/GlobalISel/legalize-extract-vector-elt.mir
llvm/test/CodeGen/AArch64/GlobalISel/legalize-load-store.mir
llvm/test/CodeGen/AArch64/GlobalISel/legalize-shuffle-vector.mir
llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-fshl.mir
llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-fshr.mir
llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-insert-vector-elt.mir
llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-llvm.amdgcn.image.load.2d.d16.ll
llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.image.load.1d.d16.ll
llvm/test/CodeGen/AMDGPU/GlobalISel/xnor.ll
|