Skip to content
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

add dexts operation #36

Merged
merged 5 commits into from
Jun 16, 2020
Merged

add dexts operation #36

merged 5 commits into from
Jun 16, 2020

Conversation

rodonisi
Copy link
Collaborator

  • the supported types are integers, vectors and signals
  • the verifier checks for type conversions and result length not being larger than the input

@rodonisi rodonisi requested a review from maerhart June 15, 2020 14:09
@maerhart
Copy link
Owner

You might want to consider to add additional class declarations to extract the slice size easily when working with this operation e.g. in passes. Something like this:

let extraClassDeclaration = [{
        unsigned getSliceWidth() {
            if (result().getType().isSignlessInteger()) {
                return result().getType().getIntOrFloatBitWidth();
            } else if (auto sigRes = result().getType().dyn_cast<llhd::SigType>()) {
               return sigRes.getUnderlyingType().getIntOrFloatBitWidth(); 
            } else if (auto vecRes = result().getType().dyn_cast<VectorType>()) {
                vecRes.getNumElements();
            }
            return 0;
        }

        unsigned getTargetWidth() {
            if (target().getType().isSignlessInteger()) {
                return target().getType().getIntOrFloatBitWidth();
            } else if (auto sigRes = target().getType().dyn_cast<llhd::SigType>()) {
               return sigRes.getUnderlyingType().getIntOrFloatBitWidth(); 
            } else if (auto vecRes = target().getType().dyn_cast<VectorType>()) {
                vecRes.getNumElements();
            }
            return 0;
        }
    }];

Then you can also use these methods to get the widths in the verify method or maybe consider traits like this:

def LLHD_DextsOp : LLHD_Op<"dexts",
        [NoSideEffect,
         PredOpTrait<"the result width cannot be larger than the target width",
            CPred<"this->getTargetWidth() <= this->getSliceWidth()">>,
         PredOpTrait<"the target and result kinds have to match",
            CPred<"$target.getType().getKind() == $result.getType().getKind()">>,
         PredOpTrait<"vector element types do not match",
            Or<[CPred<"!$result.getType().isa<VectorType>()">, TCresVTEtIsSameAsOp<0,0>]>>
        ]> { ... }

@jmgorius
Copy link
Collaborator

You might want to consider to add additional class declarations to extract the slice size easily when working with this operation e.g. in passes. Something like this:

let extraClassDeclaration = [{
        unsigned getSliceWidth() {
            if (result().getType().isSignlessInteger()) {
                return result().getType().getIntOrFloatBitWidth();
            } else if (auto sigRes = result().getType().dyn_cast<llhd::SigType>()) {
               return sigRes.getUnderlyingType().getIntOrFloatBitWidth(); 
            } else if (auto vecRes = result().getType().dyn_cast<VectorType>()) {
                vecRes.getNumElements();
            }
            return 0;
        }

        unsigned getTargetWidth() {
            if (target().getType().isSignlessInteger()) {
                return target().getType().getIntOrFloatBitWidth();
            } else if (auto sigRes = target().getType().dyn_cast<llhd::SigType>()) {
               return sigRes.getUnderlyingType().getIntOrFloatBitWidth(); 
            } else if (auto vecRes = target().getType().dyn_cast<VectorType>()) {
                vecRes.getNumElements();
            }
            return 0;
        }
    }];

Then you can also use these methods to get the widths in the verify method or maybe consider traits like this:

def LLHD_DextsOp : LLHD_Op<"dexts",
        [NoSideEffect,
         PredOpTrait<"the result width cannot be larger than the target width",
            CPred<"this->getTargetWidth() <= this->getSliceWidth()">>,
         PredOpTrait<"the target and result kinds have to match",
            CPred<"$target.getType().getKind() == $result.getType().getKind()">>,
         PredOpTrait<"vector element types do not match",
            Or<[CPred<"!$result.getType().isa<VectorType>()">, TCresVTEtIsSameAsOp<0,0>]>>
        ]> { ... }

Just nitpicking here: if you go for such an approach, I would strongly recommend you assign result.getType() and target.getType() to a local to avoid potentially redundant and deep function calls.

@maerhart
Copy link
Owner

Right, that would be a nice optimization.

@rodonisi
Copy link
Collaborator Author

Thanks for the feedback!
I moved the verification to predicate traits as suggested. The TCresVTEtIsSameAsOp though seems only to work if the checked types are explicitly VectorTypes, and I couldn't see a way have general Types casted for that check (correct me if I'm wrong). Instead I added two additional class declarations that return the element type if the type is a vector, or return null. I think that this could also come handy in other situations.

rodonisi and others added 2 commits June 16, 2020 11:41
* Added a trait to verify the types with smaller overhead
* This trait can also be used to verify the inss operation
* Deleted helper functions
* Adjusted error messages in the tests
@maerhart
Copy link
Owner

Thanks for the feedback!
I moved the verification to predicate traits as suggested. The TCresVTEtIsSameAsOp though seems only to work if the checked types are explicitly VectorTypes, and I couldn't see a way have general Types casted for that check (correct me if I'm wrong). Instead I added two additional class declarations that return the element type if the type is a vector, or return null. I think that this could also come handy in other situations.

Thanks for adding the extra class declarations!

TCresVTEtIsSameAsOp uses the getElementTypeOrSelf method defined in TypeUtilities.h. You need to add #include "mlir/IR/TypeUtilities.h" to LLHDOps.cpp to make it work.

  • I added a trait that checks the types in one go and can also be used for the llhd.inss instruction. The overhead should be smaller than in the current implementation.
  • I deleted the vectorElementTypeOrNull methods because I think they are not of much use outside of getting the trait to work. Because if you just want to get the element type if it's a vector you can call target().getType().cast<VectorType>() directly if you already know it's a vector and otherwise you need to either check that it is a vector type or check that the extra class declaration didn't return NULL which makes no difference. But if you have a use-case for it, I can add it again.

@rodonisi
Copy link
Collaborator Author

Thanks for the feedback!
I moved the verification to predicate traits as suggested. The TCresVTEtIsSameAsOp though seems only to work if the checked types are explicitly VectorTypes, and I couldn't see a way have general Types casted for that check (correct me if I'm wrong). Instead I added two additional class declarations that return the element type if the type is a vector, or return null. I think that this could also come handy in other situations.

Thanks for adding the extra class declarations!

TCresVTEtIsSameAsOp uses the getElementTypeOrSelf method defined in TypeUtilities.h. You need to add #include "mlir/IR/TypeUtilities.h" to LLHDOps.cpp to make it work.

  • I added a trait that checks the types in one go and can also be used for the llhd.inss instruction. The overhead should be smaller than in the current implementation.
  • I deleted the vectorElementTypeOrNull methods because I think they are not of much use outside of getting the trait to work. Because if you just want to get the element type if it's a vector you can call target().getType().cast<VectorType>() directly if you already know it's a vector and otherwise you need to either check that it is a vector type or check that the extra class declaration didn't return NULL which makes no difference. But if you have a use-case for it, I can add it again.

Nice addition, thank you!
I was thinking at use cases where you would do something specific with the element type if the extraction is on vectors, I have no specific example for that though. The idea was to enable something like

if (auto elemTy = dexts. getSliceVectorElementTypeOrNull()) {
    ...
}

which would be simpler than checking the cast though not by much.
All in all it was mostly a solution to make it work and that could maybe become handy later, but not necessarily. I think we can leave it out.

@rodonisi
Copy link
Collaborator Author

Also thank you for clarifying about the getElementTypeOrSelf, at first I thought that to be specific to VectorType

@maerhart maerhart merged commit 5d1f774 into master Jun 16, 2020
@maerhart maerhart deleted the add-dexts-op branch June 16, 2020 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants