-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[RISCV] Improve casting between i1 scalable vectors and i8 fixed vectors for -mrvv-vector-bits #139190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RISCV] Improve casting between i1 scalable vectors and i8 fixed vectors for -mrvv-vector-bits #139190
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1366,19 +1366,29 @@ static llvm::Value *CreateCoercedLoad(Address Src, llvm::Type *Ty, | |
| // If we are casting a fixed i8 vector to a scalable i1 predicate | ||
| // vector, use a vector insert and bitcast the result. | ||
| if (ScalableDstTy->getElementType()->isIntegerTy(1) && | ||
| ScalableDstTy->getElementCount().isKnownMultipleOf(8) && | ||
| FixedSrcTy->getElementType()->isIntegerTy(8)) { | ||
| ScalableDstTy = llvm::ScalableVectorType::get( | ||
| FixedSrcTy->getElementType(), | ||
| ScalableDstTy->getElementCount().getKnownMinValue() / 8); | ||
| llvm::divideCeil( | ||
| ScalableDstTy->getElementCount().getKnownMinValue(), 8)); | ||
| } | ||
| if (ScalableDstTy->getElementType() == FixedSrcTy->getElementType()) { | ||
| auto *Load = CGF.Builder.CreateLoad(Src); | ||
| auto *PoisonVec = llvm::PoisonValue::get(ScalableDstTy); | ||
| llvm::Value *Result = CGF.Builder.CreateInsertVector( | ||
| ScalableDstTy, PoisonVec, Load, uint64_t(0), "cast.scalable"); | ||
| if (ScalableDstTy != Ty) | ||
| Result = CGF.Builder.CreateBitCast(Result, Ty); | ||
| ScalableDstTy = cast<llvm::ScalableVectorType>(Ty); | ||
| if (ScalableDstTy->getElementType()->isIntegerTy(1) && | ||
| !ScalableDstTy->getElementCount().isKnownMultipleOf(8) && | ||
| FixedSrcTy->getElementType()->isIntegerTy(8)) | ||
| ScalableDstTy = llvm::ScalableVectorType::get( | ||
| ScalableDstTy->getElementType(), | ||
| llvm::alignTo<8>( | ||
| ScalableDstTy->getElementCount().getKnownMinValue())); | ||
| if (Result->getType() != ScalableDstTy) | ||
| Result = CGF.Builder.CreateBitCast(Result, ScalableDstTy); | ||
| if (Result->getType() != Ty) | ||
| Result = CGF.Builder.CreateExtractVector(Ty, Result, uint64_t(0)); | ||
| return Result; | ||
| } | ||
| } | ||
|
|
@@ -1476,8 +1486,14 @@ CoerceScalableToFixed(CodeGenFunction &CGF, llvm::FixedVectorType *ToTy, | |
| // If we are casting a scalable i1 predicate vector to a fixed i8 | ||
| // vector, first bitcast the source. | ||
| if (FromTy->getElementType()->isIntegerTy(1) && | ||
| FromTy->getElementCount().isKnownMultipleOf(8) && | ||
| ToTy->getElementType() == CGF.Builder.getInt8Ty()) { | ||
| if (!FromTy->getElementCount().isKnownMultipleOf(8)) { | ||
| FromTy = llvm::ScalableVectorType::get( | ||
| FromTy->getElementType(), | ||
| llvm::alignTo<8>(FromTy->getElementCount().getKnownMinValue())); | ||
| llvm::Value *ZeroVec = llvm::Constant::getNullValue(FromTy); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't sure of the semantics of bitcasting poison elements to a larger element type. Does it poison just the bits or the whole element?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The LangRef for bitcast says "It is always a no-op cast because no bits change with this conversion.", which suggests any non-poison bits must be preserved.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the original RFC for poison had this text https://lists.llvm.org/pipermail/llvm-dev/2016-October/106182.html I'm not sure if that's the semantics we ended up with. If those are the semantics then, i1 poison elements that get added to the upper bits would poison the entire i8 element after the bitcast. @nikic @nunoplopes what are the semantics of bitcasting poison elements to a larger element type?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's the semantics. A single poison bits taints the whole value.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's a choice between the LangRef and an old mailing list post then I think the LangRef should win. At least the bitcast part of the LangRef suggests this is not true, which is backed up by https://godbolt.org/z/n67WTcf1v that shows effort is put in to preserving the known bits?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is just a missed optimization. Offering bit-level poison semantics is too complicated. No one managed to come up with a proposal for that. So, we have to go with value-wise poison semantics for the foreseeable future.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You say "a missed optimization", I say "it has implemented the LangRef" :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nunoplopes is correct, there is no bitwise poison in LLVM. That particular bit of phrasing in the bitcast docs probably predates the introduction of poison. |
||
| V = CGF.Builder.CreateInsertVector(FromTy, ZeroVec, V, uint64_t(0)); | ||
| } | ||
| FromTy = llvm::ScalableVectorType::get( | ||
| ToTy->getElementType(), | ||
| FromTy->getElementCount().getKnownMinValue() / 8); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.