This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Constant fold G_PTR_ADD
ClosedPublic

Authored by arsenm on Mar 21 2021, 7:03 AM.

Details

Summary

Some globals lower to literal addresses on AMDGPU.

This may be wrong for non-integral address spaces. I'm wondering if we
should just allow regular G_ADD to use pointer types, and reserve
G_PTR_ADD for non-integral address spaces.

Diff Detail

Event Timeline

arsenm created this revision.Mar 21 2021, 7:03 AM
arsenm requested review of this revision.Mar 21 2021, 7:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2021, 7:03 AM
Herald added a subscriber: wdng. · View Herald Transcript
paquette accepted this revision.Mar 23 2021, 9:53 AM

LGTM

I'm wondering if we should just allow regular G_ADD to use pointer types, and reserve G_PTR_ADD for non-integral address spaces.

Maybe? We've had a few cases with AArch64 where we miss optimizations because we forget to handle both G_ADD and G_PTR_ADD.

I guess that knowing immediately that we have a pointer add is nice for matching etc, but I guess we also know that from the LLT.

This revision is now accepted and ready to land.Mar 23 2021, 9:53 AM

Relaxing G_ADD semantics seems fine to me. Maybe we should still canonicalize on G_PTR_ADD during translation, and allow targets to legalize them into plain G_ADDs.

Relaxing G_ADD semantics seems fine to me. Maybe we should still canonicalize on G_PTR_ADD during translation, and allow targets to legalize them into plain G_ADDs.

IRTranslator should definitely translate to G_PTR_ADD and G_PTR_ADD should not be considered equivalent to G_ADD. However for targets where pointers are just integers I see no harm in permitting G_ADD of pointers and targets custom-legalizing/lowering to that. It just shouldn't be something that happens without the targets actively choosing to do it.

llvm/lib/CodeGen/GlobalISel/Utils.cpp
503

I don't think we should add constant folding for G_PTR_ADD since it might not be an integer and might even be addrspace dependent behaviour. For integral pointers we should lower to G_ADD first, and then use G_ADD's constant folding.

arsenm updated this revision to Diff 406989.Feb 8 2022, 3:01 PM

Filter out non-integral address spaces for now, although the general handling needs more thought

arsenm requested review of this revision.Feb 8 2022, 3:01 PM
arsenm updated this revision to Diff 406998.Feb 8 2022, 3:28 PM

Move handling to potentially catch vector case

paquette accepted this revision.Feb 8 2022, 3:30 PM

Filtering out non-integral spaces for now makes sense to me.

llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
208

probably worth a comment explaining that this needs more thought?

This revision is now accepted and ready to land.Feb 8 2022, 3:30 PM
arsenm added inline comments.Feb 8 2022, 4:05 PM
llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
208

It's not really a property of right here, it's just nobody has seriously tried to implement these at all