This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Handle ptr size != index size in IRTranslator, CodeGenPrepare
ClosedPublic

Authored by krzysz00 on Feb 7 2023, 1:49 PM.

Details

Summary

While the original motivation for this patch (address space 7 on
AMDGPU) has been reworked and is not presently planned to reach IR
translation, the incorrect (by the spec) handling of index offset
width in IR translation and CodeGenPrepare is likely to trip someone

  • possibly future AMD, since we have a p7:160:256:256:32 now, so we

convert to the other API now.

Diff Detail

Event Timeline

krzysz00 created this revision.Feb 7 2023, 1:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 1:49 PM
krzysz00 requested review of this revision.Feb 7 2023, 1:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 1:49 PM
aemerson accepted this revision.Feb 7 2023, 1:55 PM
This revision is now accepted and ready to land.Feb 7 2023, 1:55 PM
arsenm added a comment.Feb 7 2023, 1:58 PM

Test coverage looks incomplete

@arsenm Do you have suggestions on what additional tests would be needed? I'm not terribly familiar with this portion of the codebase.

arsenm added inline comments.Feb 8 2023, 10:31 AM
llvm/lib/CodeGen/CodeGenPrepare.cpp
6063

Some addressing mode thing, probably would be best to hack up a test that hits an unreachable here

6093

Ditto for these other CGP parts, which are really separate from the IRTranslator part

llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
850

this would be an ptr addrspace(7) at the end of a large struct return value, on the caller side

879

this would be an ptr addrspace(7) at the end of a large struct return value

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1516

vector GEP with a scalar index?

llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-non-integral-address-spaces.ll
20

Some vector GEP cases

krzysz00 updated this revision to Diff 496570.Feb 10 2023, 11:35 AM

Wherein I noticed getIndexType() exists, cascading rebase

llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
879

Having looked at the context, I think we'd have to invent a target to test this. That is, we'd need a target where the stack lives in a intptr_t > size_t world, which ... well, that's CHERI, but not anything upstream.

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1516

I figure that already either worked or didn't?

arsenm added inline comments.Feb 10 2023, 12:26 PM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1516

There certainly isn't a test using a fat pointer using this

krzysz00 updated this revision to Diff 496592.Feb 10 2023, 1:36 PM

Added missing test

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1516

Good point, added.

arsenm accepted this revision.Feb 10 2023, 4:32 PM

GlobalISel part lgtm but the cgp part should be split out

llvm/lib/CodeGen/CodeGenPrepare.cpp
6063

CGP part is still separate and separately testable

krzysz00 updated this revision to Diff 519910.May 5 2023, 10:01 AM
krzysz00 retitled this revision from [GlobalISel] Handle ptr size != index size in IRTranslator to [GlobalISel] Handle ptr size != index size in IRTranslator, CodeGenPrepare.
krzysz00 edited the summary of this revision. (Show Details)

Rebase, remove updates to a test that doesn't exist anymore.

Annoyingly, since llc pulls in the data layout, there's not currently good ways to test this that live in the repo.

krzysz00 updated this revision to Diff 521682.May 12 2023, 9:16 AM

Now we have a test

This revision was landed with ongoing or failed builds.May 12 2023, 9:21 AM
This revision was automatically updated to reflect the committed changes.