This is an archive of the discontinued LLVM Phabricator instance.

[GlobalIsel][X86] Legalize G_CTPOP and G_CTLZ
ClosedPublic

Authored by tschuett on May 16 2023, 7:59 AM.

Details

Summary

G_BSWAP was reverted -> added to this diff.

check plan: ninja check-llvm-codegen-x86

Future work: G_SUB and G_ZEXT need some modernization.

Diff Detail

Event Timeline

tschuett created this revision.May 16 2023, 7:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 7:59 AM
tschuett requested review of this revision.May 16 2023, 7:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 7:59 AM
tschuett edited the summary of this revision. (Show Details)May 16 2023, 8:00 AM
arsenm added inline comments.May 16 2023, 8:02 AM
llvm/lib/Target/X86/X86LegalizerInfo.cpp
402

These both have buggy type lists, and I'm surprised nothing is blowing up. These have 2 type parameters, the result may be different from the source

arsenm added inline comments.May 16 2023, 8:03 AM
llvm/lib/Target/X86/X86LegalizerInfo.cpp
402

Now, he fails to legalise:
unable to legalize instruction: %3:_(s16) = G_CTLZ %0:_(s8) (in function: test_ctlz8)
Note s8 and s16.

tschuett added inline comments.May 16 2023, 8:37 AM
llvm/test/CodeGen/X86/GlobalISel/legalize-leading-zeros.mir
44

The input is s8 and the output is s16.

arsenm added inline comments.May 16 2023, 8:44 AM
llvm/test/CodeGen/X86/GlobalISel/legalize-leading-zeros.mir
44

I think you're running into the over-constrained API problem. I think it only works as you would expect if you legalize the first operand and the second operand second. The current API cannot express your desired changes for both types at the same time, and the LegalizerHelper currently doesn't handle arbitrary requests.

Any hints how to implement your approach?

Any hints how to implement your approach?

Which approach? Not sure precisely what you are trying to achieve in the example. I assume you want s8 to promote both operands? The -debug log should show the legalization steps it's trying to take

G_CTLZ is only legal for s16, s32, and s64. I was hoping that it will somehow legally widened to s16 ops.

G_CTLZ is only legal for s16, s32, and s64. I was hoping that it will somehow legally widened to s16 ops.

Your legalize rules aren't constraining the result and source type to match though. They're expressing the constraints as though they are independently chosen

tschuett updated this revision to Diff 522730.May 16 2023, 11:39 AM

changed the type of clampScalar

arsenm accepted this revision.May 16 2023, 12:18 PM
arsenm added inline comments.
llvm/lib/Target/X86/X86LegalizerInfo.cpp
403–404

Should probably swap these lines but it doesn't make much difference. The API could also use work for the common clamp-to-power-of-2-in-range case

This revision is now accepted and ready to land.May 16 2023, 12:18 PM

Thanks. I am only looking for low hanging fruit and not to maximize coverage. X86 Gisel fails fast and loud.

This revision was landed with ongoing or failed builds.May 16 2023, 1:14 PM
This revision was automatically updated to reflect the committed changes.