Page MenuHomePhabricator

Non-8-bit bytes showcase
Needs ReviewPublic

Authored by JesperAntonsson on May 2 2019, 4:48 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

This patch set is meant to show the key addition of a BitsPerByte member of DataLayout and how this can be used in refactorings to remove magic numbers related to the assumption of 8-bit bytes.

Only a small number of such refactorings are shown, but should convey what they'd look like. An RFC will be sent to the llvm community with a suggestion for a way forward.

Diff Detail

Event Timeline

JesperAntonsson created this revision.May 2 2019, 4:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 4:48 AM
arsenm added a subscriber: arsenm.May 2 2019, 4:53 AM
arsenm added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
215

I believe this should be readonly

20005–20006

Should introduce a rounded size in bytes function somewhere

uabelho added a subscriber: uabelho.May 2 2019, 5:27 AM
riccibruno added inline comments.
include/llvm/IR/DataLayout.h
421

That's an expensive division...

probinson added inline comments.
include/llvm/IR/DataLayout.h
421

Maybe derive a pointer-type-size-in-bytes calculated in the ctor.
Or, as a more generic solution, derive shift/mask values from bits-per-byte, so the refactoring can replace multiply/divide with shifts. Or maybe with bitsToBytes and bytesToBits helper methods.

psnobl added a subscriber: psnobl.May 2 2019, 9:52 AM
Ka-Ka added a subscriber: Ka-Ka.May 7 2019, 12:26 AM

Added an inBytes() helper method. (We could contemplate putting this in DataLayout instead for more general use.) Adjusted READNONE to READONLY. Improved comments.

Will make a twin patch with a suggestion for "addressable units" terminology.

JesperAntonsson marked 2 inline comments as done.May 9 2019, 5:12 AM
JesperAntonsson added inline comments.
include/llvm/IR/DataLayout.h
421

Yes, or we could perhaps make this a static constant in DataLayout and, for now, leave it to downstream target to make it a variable.

Daniel added a subscriber: Daniel.May 18 2019, 2:01 PM
Daniel added inline comments.May 18 2019, 2:20 PM
include/llvm/IR/DataLayout.h
421

This looks fine as long as performance doesn't get worse after refactor. I don't see a performance hit here.