Page MenuHomePhabricator

[DataLayout] Add bit width of pointers to global values
AbandonedPublic

Authored by nhaehnle on Oct 11 2018, 12:29 PM.

Details

Summary

Together with leading known-zero bit tracking for getelementptr, this
recovers most of the code quality that was lost for AMDGPU's SI subtarget
with a previous commit.

AMDGPU has LDS (local) and GDS (region) address spaces that use 32 bit
pointers, even though the underlying memory is only 64 kiB, and therefore
pointers to global values always fit into 16 bits.

This recovers a lot of the code quality lost in "AMDGPU: Restrict DS
load/store vectorizing on SI".

Change-Id: Ic17f297b714039ea4988581b3abfafc57d11a6bd

Diff Detail

Event Timeline

nhaehnle created this revision.Oct 11 2018, 12:29 PM

Can you use the "address space" to distinguish between global and local in Data Layout?

bjope added a subscriber: bjope.Oct 12 2018, 2:42 AM
bjope added inline comments.
include/llvm/IR/DataLayout.h
340

Maybe remove the default here. There are comments about removing AS defaults for other methods here, and usually when someone does not provide the address space in the call it could be a hard to find fault.

theraven added inline comments.Oct 12 2018, 3:22 AM
include/llvm/IR/DataLayout.h
340

Agreed. The default value is useful for incrementally migrating consumers of APIs that didn't know about address spaces, but for a new API it doesn't make sense.

lib/Analysis/ValueTracking.cpp
1344

Does this correctly handle the case of invalid pointers being created as temporary values? In CHERI, we saw a few cases where, after reassociation, you ended up with IR that took a pointer out of bounds in one calculation and then brought it back in as a result of the next one before dereferencing. This should perhaps be restricted to inbounds GEPs (or is it already and I'm missing the check)?

lib/IR/DataLayout.cpp
311

Please can we have a comment that we're assuming 8-bit bytes here? A few out-of-tree targets are trying to make things work with different addressable sizes and it's polite to leave them a signpost that this is a thing that they'll need to change.

nhaehnle updated this revision to Diff 169367.Oct 12 2018, 3:58 AM
nhaehnle marked 3 inline comments as done.
  • no default value for address space
  • add comment about 8-bit bytes

Thank you all for taking a look.

Can you use the "address space" to distinguish between global and local in Data Layout?

We may be talking past each other. This is about GlobalValues (i.e. global variables, e.g. file-level variables in C/C++), not about global vs. local. Those could exist in any address space.

include/llvm/IR/DataLayout.h
340

That makes a lot of sense, thanks for the history lesson :)

lib/Analysis/ValueTracking.cpp
1344

The intention of this code is to just not care about the in- or out-of-boundedness of GEPs, so yes, it should handle temporary invalid pointers correctly (though maybe over-conservatively).

To think this through:

  • GEP indices are always supposed to be treated as signed integers. If the index may be negative, setting LeadZ = 0 is conservatively correct: we make no claim about leading zeros at all anymore in this case.
  • Zero indices don't actually change the pointer value, so need no change of LeadZ.
  • Non-zero but positive indices change the pointer value by adding some offset. The intuition behind the code here is that the base pointer (or rather, the base pointer plus offsets from earlier GEP indices) fits into some number of bits, and the offset that we're adding in this GEP index fits into some number of bits, so their some fits into max(those numbers) + 1. Though the code itself is expressed in number of leading zeros as opposed to number of bits needed to represent the (unsigned) pointer value.

It seems like some tighter guarantees could be made for inbounds GEPs, but this code does not attempt that.

Does that make sense to you?

theraven added inline comments.Oct 12 2018, 4:58 AM
lib/Analysis/ValueTracking.cpp
1344

Sounds good, thanks for the clarification.

I'm not sure it makes sense to put this information in the DataLayout.

The number of bits required to represent the address of a global isn't really a fundamental aspect of the target; it can vary based on the code generation options used. For example, on x86-64, non-position-independent code can assume the address of a global fits into 32 bits, but we don't want to change the datalayout based on whether the user passed -fPIC. And even if it is fundamental on some targets, it doesn't seem important enough to include in the datalayout; no target-independent transforms care about known bits for pointers beyond figuring out the alignment of a pointer.

no target-independent transforms care about known bits for pointers beyond figuring out the alignment of a pointer.

The other patch in this stack (D53160) cares about it. And sure, that patch is target-dependent, but it's based on the target-independent computeKnownBits. The canonical way that this kind of information is passed into computeKnownBits is via the DataLayout. The alternative would be to start passing some separate target-info into computeKnownBits, which seems way uglier to me.

So yeah, I hear your complaint, but unless you can come up with a constructive alternative, this patch is the way to go.

I also think this is pretty weird for the datalayout. An optional TTI parameter that improves computeKnownBits seems natural to me? We could also handle target intrinsics more easily with that

nhaehnle abandoned this revision.Oct 17 2018, 5:18 AM

I'm dropping this change. We don't need it anymore, given changes to the parent revision.