This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] Make G_PTRTOINT only legal for s64 + p0
ClosedPublic

Authored by paquette on Aug 5 2021, 11:47 AM.

Details

Summary

A few issues:

  1. There was no legalizer test for G_PTRTOINT
  2. Same clamping issue as in many other opcodes
  3. AArch64 pointers can only be 64b, so in reality we always have to trunc or extend with any size other than p0 anyway.

This seems to actually produce more correct selection for narrow types as well.

Diff Detail

Event Timeline

paquette created this revision.Aug 5 2021, 11:47 AM
paquette requested review of this revision.Aug 5 2021, 11:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2021, 11:47 AM

I think the clamping behavior should only allow s64 result types, since AArch64 pointers can only be 64b.

paquette updated this revision to Diff 364634.Aug 5 2021, 3:25 PM

Always clamp to s64

aemerson added inline comments.Aug 5 2021, 3:36 PM
llvm/test/CodeGen/AArch64/GlobalISel/legalize-ptrtoint.mir
27

I expected these to change?

paquette added inline comments.Aug 5 2021, 3:53 PM
llvm/test/CodeGen/AArch64/GlobalISel/legalize-ptrtoint.mir
27

The existing legality behaviour says it's okay:

.legalForCartesianProduct({s1, s8, s16, s32, s64}, {p0})

Should we just drop everything except for s64 entirely?

aemerson added inline comments.Aug 5 2021, 4:06 PM
llvm/test/CodeGen/AArch64/GlobalISel/legalize-ptrtoint.mir
27

Yes I think so.

paquette updated this revision to Diff 364650.Aug 5 2021, 4:37 PM
paquette retitled this revision from [AArch64][GlobalISel] Widen G_PTRTOINT before clamping + test legalization to [AArch64][GlobalISel] Make G_PTRTOINT only legal for s64 + p0.
paquette edited the summary of this revision. (Show Details)

Only allow s64.

I think that, running through to selection, this actually fixes a couple bugs too. (ptrtoint_s16_p0 and ptrtoint_s8_p0 were using sub_32???)

aemerson accepted this revision.Aug 5 2021, 4:50 PM

LGTM.

This revision is now accepted and ready to land.Aug 5 2021, 4:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 4:22 PM