This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Use GCDTy when extracting GCD ty from leftover regs in insertParts
ClosedPublic

Authored by paquette on Jul 8 2021, 5:16 PM.

Details

Summary

LegalizerHelper::insertParts uses extractGCDType on registers split into a desired type and a smaller leftover type. This is used to populate a list of registers. Each register in the list will have the same type as returned by extractGCDType.

If we have

  • ResultTy = s792
  • PartTy = s64
  • LeftoverTy = s24

When we call extractGCDType, we'll end up with two different types appended to the list:

Part: gcd(792, 64, 24) => s8
Leftover: gcd(792, 24, 24) => s24

When this happens, we'll hit an assert while trying to build a G_MERGE_VALUES.

This patch changes the code for the leftover type so that we reuse the GCD from the desired type.

e.g.

Leftover: gcd(792, 8, 24) => s8

It also adds an assert that the types we get from both calls to extractGCDType are actually the same.

https://llvm.godbolt.org/z/137Kqxj6j

Diff Detail

Event Timeline

paquette created this revision.Jul 8 2021, 5:16 PM
paquette requested review of this revision.Jul 8 2021, 5:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2021, 5:16 PM
Herald added a subscriber: wdng. · View Herald Transcript
foad added a subscriber: foad.Jul 9 2021, 3:41 AM

It would be simpler and more efficient to calculate GCDType once before both "for" loops (copy the code for this out of the 4 argument extractGCDType), and then use the 3 argument extractGCDType inside both "for" loops.

paquette updated this revision to Diff 357538.Jul 9 2021, 9:12 AM

Use 3-argument variant of extractGCDType to avoid recalculating GCDTy

foad added inline comments.Jul 9 2021, 11:57 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
229

I was thinking of just writing out LLT GCDTy = getGCDType(getGCDType(..., ...), ...); here to avoid having to do the first reg separately.

paquette updated this revision to Diff 357622.Jul 9 2021, 1:38 PM
  • Don't do the first part separately
  • Use concat to write one loop instead of two
arsenm accepted this revision.Jul 9 2021, 2:07 PM
This revision is now accepted and ready to land.Jul 9 2021, 2:07 PM
This revision was landed with ongoing or failed builds.Jul 9 2021, 2:16 PM
This revision was automatically updated to reflect the committed changes.
foad added a comment.Jul 9 2021, 2:32 PM
  • Don't do the first part separately
  • Use concat to write one loop instead of two

Much better, thanks!