This is an archive of the discontinued LLVM Phabricator instance.

Make ConstantDataArray::get constructor templated. Will support signed integers.
ClosedPublic

Authored by asbirlea on Mar 9 2018, 4:27 PM.

Diff Detail

Event Timeline

asbirlea created this revision.Mar 9 2018, 4:27 PM
timshen added inline comments.Mar 9 2018, 4:43 PM
include/llvm/IR/Constants.h
706

Would it be good to create Type::getTyByScalar:

class Type {
  template <typename ScalarType>
  Type* getTyByScalar(LLVMContext*) {
    switch (sizeof(ScalarType)) { ... }
  }

Also, it'd be better to expand getTyByScalar to pointer and all sorts of other crazy types, but Scalar would be a good start.

Then ConstantDataArray::get will be like

return ArrayType::get(Type::getTyByScalar<ElementTy>(Context), Elts.size());
sanjoy added a comment.Mar 9 2018, 5:23 PM

Minor drive by comments.

include/llvm/IR/Constants.h
701

I'm not sure of the rules here, but perhaps this needs to be sizeof(ElementTy) * CHAR_BIT / 8 to be technically correct?

708

There is a Type::getIntNTy that you can use here.

732

Use llvm_unreachable.

asbirlea updated this revision to Diff 138028.Mar 12 2018, 8:56 AM
asbirlea marked 3 inline comments as done.

Address comments.

include/llvm/IR/Constants.h
701

It made sense to add * CHAR_BIT when passing no of bits to getIntNTy.
But having no_bytes = sizeof * CHAR_BIT / 8, and no bits = no_bytes *CHAR_BIT together doesn't make sense. I dropped the first, but I'm not clear if that's correct.

asbirlea updated this revision to Diff 138052.Mar 12 2018, 10:33 AM

Switch to noOfBits in Type.h.

timshen accepted this revision.Mar 19 2018, 10:40 AM

LGTM with some nits.

include/llvm/IR/Constants.h
704

Maybe just Elts.size() * sizeof(ElementTy)?

include/llvm/IR/Type.h
410

getScalarTy seems a more consistent name.

This revision is now accepted and ready to land.Mar 19 2018, 10:40 AM
asbirlea updated this revision to Diff 138974.Mar 19 2018, 12:00 PM

Address comments.
Thank you for the review!

This revision was automatically updated to reflect the committed changes.