This is an archive of the discontinued LLVM Phabricator instance.

Allow DataLayout to support arbitrary pointer sizes
ClosedPublic

Authored by stephenneuendorffer on Nov 17 2021, 11:33 PM.

Details

Summary

Currently, it is impossible to specify a DataLayout with pointer
size and index size that is not a whole number of bytes.
This patch modifies
the DataLayout class to accept arbitrary pointer sizes and to
store the size as a number of bits, rather than as a number of bytes.
Generally speaking, the external interface of the class as used
by in-tree architectures remains the same and shouldn't affect the
behavior of architecures with pointer sizes equal to a whole number
of bytes.

Note the interface of setPointerAlignment has changed and takes
a pointer and index size that is a number of bits, rather than a number
of bytes.

Patch originally by Ajit Kumar Agarwal

Diff Detail

Event Timeline

stephenneuendorffer requested review of this revision.Nov 17 2021, 11:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 11:33 PM
jrtc27 added inline comments.Nov 18 2021, 10:19 AM
llvm/lib/IR/DataLayout.cpp
321

These shouldn't need an explicit template parameter, it's inferable from PointerMemSize's type?

714

divideCeil from MathExtras?

llvm/lib/Target/M68k/M68kISelLowering.cpp
173

These aren't really necessary. I'd suggest separating out the "add more flexibility to DataLayout" changes from the "simplify users of existing interfaces by using new ones" changes so it's clear there isn't actually an API break here.

stephenneuendorffer marked 2 inline comments as done.
stephenneuendorffer marked an inline comment as done.
stephenneuendorffer edited the summary of this revision. (Show Details)
tschuett added inline comments.
llvm/include/llvm/IR/DataLayout.h
101–102

This is dangerous. You are changing the meaning without changing the names?!? Maybe 2 versions of get?

tschuett added inline comments.Dec 2 2021, 12:55 AM
llvm/include/llvm/IR/DataLayout.h
183

Same here. Maybe keep the old functions and only add new functions with ìnBits in the name.

llvm/include/llvm/IR/DataLayout.h
183

True, but this seems like a private interface? I'd be more inclined to change the name without keeping the old function.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 7 2021, 11:20 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
stephenneuendorffer marked 2 inline comments as done.

Not sure how you can construct a test that triggers this problem, but in our out-of-tree version, this triggered problems.

llvm/lib/IR/DataLayout.cpp
714

Shouldn't this be .....divideCeil(P.IndexBitWidth, 8) ?

uabelho added inline comments.Dec 14 2021, 12:30 AM
llvm/lib/IR/DataLayout.cpp
714

ping2 @stephenneuendorffer : could you check and comment on P.TypeBitWidth vs P.IndexBitWidth on line 715 ?