This is an archive of the discontinued LLVM Phabricator instance.

[llvm][IR][CastInst] Update `castIsValid` for scalable vectors.
ClosedPublic

Authored by fpetrogalli on Mar 24 2020, 3:52 PM.

Diff Detail

Event Timeline

fpetrogalli created this revision.Mar 24 2020, 3:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2020, 3:52 PM

I have updated the whole method CastInst::castISValid to correctly
support scalable vectors.

fpetrogalli retitled this revision from [llvm] Use correct type in variable declaration. [NFC] to [llvm][IR][CastInst] Update `castIsValid` for scalable vectors..Mar 25 2020, 8:15 PM
fpetrogalli edited the summary of this revision. (Show Details)
fpetrogalli added a reviewer: efriedma.EditedMar 26 2020, 12:29 PM
This comment has been deleted.
efriedma added inline comments.Mar 26 2020, 12:39 PM
llvm/lib/IR/Instructions.cpp
3241

What's wrong with getScalarSizeInBits()?

3283–3284

Can we simplify some of the getElementCount() calls by using SrcLength/DstLength instead?

fpetrogalli marked 3 inline comments as done.Mar 26 2020, 2:25 PM
fpetrogalli added inline comments.
llvm/lib/IR/Instructions.cpp
3240–3245

Nit: While doing some of the modification you have asked me, I got confused by the fact that the variables [Src|Dst]BitSize are referring to the size of the scalars, not of the whole [Src|Dst]Ty. Is it OK if I rename them to [Src|Dst]ScalarBitSize?

3241

getScalarSizeInBits implicitly casts the TypeSize of getScalarType()->getPrimitiveSizeInBits() into an integer, an operation that we have agreed to mark as deprecated. I probably should have mentioned this in the commit message.

getScalarSizeInBits is implemented as follows:

unsigned Type::getScalarSizeInBits() const {
  return getScalarType()->getPrimitiveSizeInBits();
}

Given that we don't expect "scalar" values to be scalable, I can revert this change to use unsigned values, and maybe submit a separate patch that adds an assertion inside getScalarSizeInBits that makes sure to check that the TypeSize retrieved by getPrimitiveSizeInBits is not a scalable TypeSize.

The method will look like this after the change (disclaimer, I didn't compile it!):

unsigned Type::getScalarSizeInBits() const {
  TypeSize TS = getScalarType()->getPrimitiveSizeInBits();
  assert(!TS.isScalable() && "Invalid type.")
  return TS.getKnownMinSize();
}

Please let me know which of the following you want me to do:

  1. mention and justify the change in the commit message
  2. revert the change to use unsigned, and create a separate patch that add the assertion "is not scalable" to getScalarSizeInBits.
  3. third option you might have that I have missed! :)
3283–3284

Can we simplify some of the getElementCount() calls by using SrcLength/DstLength instead?

Yes, on it.

How about also creating two boolean vars that hold the isa<VectorType>([Src|Dst]Ty) and use them instead of the isa<VectorType>([Src|Dst]Ty) or dyn_cast<VectorType>([Src|Dst]Ty).?

efriedma added inline comments.Mar 26 2020, 3:13 PM
llvm/lib/IR/Instructions.cpp
3240–3245

Sure.

3241

I think it makes sense to keep using getScalarSizeInBits(), and fix the implementation like you suggest.

You can use getFixedSize() instead of separately asserting !isScalable().

3283–3284

Whatever you think is more readable is fine.

Hi @efriedma,

thank you for your review!

I have addressed all your comments, and applied the following changes:

  1. restored the use of unsigned for the size in bits. I have created a

separate patch that updated the unsigned Type::getScalarSizeInBits()
to use TypeSize::getFixedSize(): https://reviews.llvm.org/D76892

  1. I have fixed the tests that were refusing to bitcast fixed size

vectors to scalable vectors (and viceversa). In the previous patch I
erroneusly used scalable vectors everywhere, just with different
lanes.

  1. I have replaced the use of getVectorElementCount with variables

that are set at the beginning of the method.

  1. I have renamed [Src|Dst]BitSize to [Src|Dst]ScalarBitSize, to

avoid confusion when dealing with vector types instead of scalars.

Kind regards,

Francesco

fpetrogalli marked 5 inline comments as done.Mar 26 2020, 4:14 PM

Last but not least, I have renamed [Src|Dst]Length into [Src|Dst]EC as the variables now hold ElementCount and not unsigned.

efriedma added inline comments.Mar 26 2020, 4:45 PM
llvm/lib/IR/Instructions.cpp
3282

"if (SrcIsVec != DstIsVec)" is redundant, here and for IntToPtr.

3328–3329

Remove "if (SrcIsVec)", and just do "return SrcEC == DstEC;"?

fpetrogalli marked 2 inline comments as done.

Hi @efriedma,

I have further simplified the code according to your comments.

Thank you!

Francesco

fpetrogalli marked an inline comment as not done.Mar 27 2020, 1:56 PM
This revision is now accepted and ready to land.Mar 27 2020, 2:21 PM

Rebasing on top of master. [NFC]

This comment was removed by fpetrogalli.

Rebase on top of master.

This revision was automatically updated to reflect the committed changes.