-
Notifications
You must be signed in to change notification settings - Fork 99
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
Differentiate between signed and unsigned types #62
Comments
Yep, that's the status quo, and this is also related with #5 Both are something we're gonna have to tackle sooner than later anyways, in case you wanna add to your list! |
@bcardosolopes regarding #5, what is the level of granularity we are looking for? Does a I suggest we mirror MLIR's built-in dialect types (single int type with arbitrary size and one float type per size), since it already tracks signedness, then add a few qualifier attributes to mark |
Tracking arbitrary sizes sounds good enough.
Signedness is pretty important because we want code analysis writers to be able to detect things like integer overflows and whatnots. If we can take advantage of the underlying in-tree primitive types to represent that, all we would need is an extra getter method for C/C++ specific signed/unsigned queries. Implementing primitive types while tracking signedness would be step (1). Step (2): we should also consider adding an optional
This brings an interesting point, qualifiers in clang are not part of the type, I believe the intent was to optimize memory usage for not creating extra types every time there's a qualifier variation (and also possibly helps to implement deductions that drop qualifiers, etc). We should probably handle qualifiers as part of step (2) or a new step (3), so we have some time to think/discuss while we make progress. Thoughts? |
Regarding step 1, using
The The built-in Floating point types seem to cover all C/C++ primitives as well. With this in mind, should we use MLIR's built-in type for C/C++ primitives instead of custom CIR types? |
Yeah I now, it's pretty attractive (I've been there).
I still believe we should wrap them so we can customize as we see fit (e.g. adding CIR specific attributes that won't be dropped by random passes) and hide the rest of CIR from MLIR in tree changes - example: if they decide at some point that the types should be part of a specific dialect, we only have to change our implementation in one specific place. It will also make our lives easier when adding qualifiers (be it by incorporating clang types with specific attributes or adding our own notion of qualifiers). |
Yup. As a first-principle we want to avoid being coupled to downstream MLIR changes. Rebasing against MLIR is an absolute nightmare. And when they change functionality of dialects/types/etc we become upwards exposed to surprise behavioral differences. I doubt At a high level, we use MLIR as an infrastructure for writing an IR and not as a tree of dialects that we can use. It would be fine as an intermediate step if we used |
Updates CodeGen type converter and emitters to handle sign information of integer values. Lowering is also updated to convert const_arrays of signed types. Most tests were also updated since MLIR uses a 's' and 'u' prefix on integer types to identify their sings. Fix llvm#62
@bcardosolopes @lanza can you take a look at this draft: It implements a custom |
#72 merged |
CIR Integer types are built without any sign information:
clangir/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
Lines 394 to 396 in 0bbac72
The text was updated successfully, but these errors were encountered: