[SYCL-PTX] Add a JSON backend for the ProgModel builtin #1781
Conversation
| } | ||
|
|
||
| std::string JSONBuiltinInterfaceEmitter::TypeDesc::GetBaseTypeAsStr() const { | ||
| if (Name == "void") { |
There was a problem hiding this comment.
Don't use curleys for single line if/else/etc. Applies through this patch.
There was a problem hiding this comment.
Is it? I see some below as well as here still.
There was a problem hiding this comment.
done, BTW I can't find where the coding style mention this.
There was a problem hiding this comment.
Apparently it is just something that has been enforced forever without having ever been put into the document! I'm likely going to add it to the document to be consistent with what reviewers enforce.
| void JSONBuiltinInterfaceEmitter::Emit() { | ||
|
|
||
| // Parse the Records to populate the internal lists. | ||
| GetOverloads(); |
There was a problem hiding this comment.
I suspect calling these directly is against the original author's abstraction (which is why they were private in the first place). How do other emitters handle this?
There was a problem hiding this comment.
There is no other emitter
Naghasan
force-pushed
the
victor/json-builtins
branch
3 times, most recently
from
31fcbc3 to
a2d8392
Compare
| // Size of the element in bits. | ||
| int Size; | ||
| // Is a integer. | ||
| bool IsInteger; |
There was a problem hiding this comment.
The hope is that it would be more readable as well as more consistent with the rest of clang code.
| } | ||
|
|
||
| std::string JSONBuiltinInterfaceEmitter::TypeDesc::GetBaseTypeAsStr() const { | ||
| if (Name == "void") { |
There was a problem hiding this comment.
Is it? I see some below as well as here still.
|
|
||
| if (Ty->isSubClassOf("GenericType")) { | ||
| assert(!Expansion.size() && "Types already expanded"); | ||
| auto NVec = Ty->getValueAsDef("VectorList")->getValueAsListOfInts("List"); |
There was a problem hiding this comment.
Throughout the patch, you're using 'auto' in cases that the coding standard prohibits.
Naghasan
force-pushed
the
victor/json-builtins
branch
from
a2d8392 to
591043a
Compare
Add a JSON output for the ClangProgModelBuiltinEmitter. This patch also include header generator for libclc that consumes the JSON ouput, which gets automatically trigged on changes on the SPIRVBuiltins.td. Signed-off-by: Victor Lomuller <[email protected]>
… rather than directly in type emission. Signed-off-by: Victor Lomuller <[email protected]>
Signed-off-by: Victor Lomuller <[email protected]>
Signed-off-by: Victor Lomuller <[email protected]>
Signed-off-by: Victor Lomuller <[email protected]>
Signed-off-by: Victor Lomuller <[email protected]>
Naghasan
force-pushed
the
victor/json-builtins
branch
from
2851cbc to
90ba58a
Compare
| IsPointer(T->getValueAsBit("IsPointer")), | ||
| IsConst(T->getValueAsBit("IsConst")), | ||
| IsVolatile(T->getValueAsBit("IsVolatile")) { | ||
| assert((!T->getValueAsBit("IsInteger") || !T->getValueAsBit("IsFloat")) && |
There was a problem hiding this comment.
Can you please add the isSubClassOf checks as well? It seems that it shouldn't be a BUILTIN either.
There was a problem hiding this comment.
It is already handled by the unreachable.
There was a problem hiding this comment.
I don't think it is. I'm talking about a type that IsInteger, IsFloat, and isSubClassOf(FundamentalType) AND isSubClassOf(VectorType).
At the moment you catch only isInteger and isFloat, but if you leave any of those off it isnt.
Additionally, the logic below allows a type to be both a fundamental and vector type.
Signed-off-by: Victor Lomuller <[email protected]>
…e a scoped enum in its scope. Signed-off-by: Victor Lomuller <[email protected]>
| IsPointer(T->getValueAsBit("IsPointer")), | ||
| IsConst(T->getValueAsBit("IsConst")), | ||
| IsVolatile(T->getValueAsBit("IsVolatile")) { | ||
| assert((!T->getValueAsBit("IsInteger") || !T->getValueAsBit("IsFloat")) && |
There was a problem hiding this comment.
I don't think it is. I'm talking about a type that IsInteger, IsFloat, and isSubClassOf(FundamentalType) AND isSubClassOf(VectorType).
At the moment you catch only isInteger and isFloat, but if you leave any of those off it isnt.
Additionally, the logic below allows a type to be both a fundamental and vector type.
| }); | ||
| assert(SignatureLenIt.begin() != SignatureLenIt.end()); | ||
| // Find out how many overloads this signature holds. | ||
| size_t NbOverload = 1; |
There was a problem hiding this comment.
| size_t NbOverload = 1; | |
| size_t NbOverload = *std::max_element(std::begin(SignatureLenIt), std::end(SignatureLenIt)); |
| std::vector<long int> NVec = | ||
| Ty->getValueAsDef("VectorList")->getValueAsListOfInts("List"); |
There was a problem hiding this comment.
Sorry for the delay.
Out internal pre-commit check failed to build this patch on Windows with the following message:
clang\utils\TableGen\ClangProgModelBuiltinEmitter.cpp(1026): error C2440: 'initializing': cannot convert from 'std::vector<int64_t,std::allocator<_Ty>>' to 'std::vector<long,std::allocator<_Ty>>'
with
[
_Ty=int64_t
]
and
[
_Ty=long
]
clang\utils\TableGen\ClangProgModelBuiltinEmitter.cpp(1026): note: No constructor could take the source type, or constructor overload resolution was ambiguous
There was a problem hiding this comment.
On Windows, "long" is 32 bit, so int64_t is long long. This likely should be std::vector<int64_t> instead.
This add the generator used to create the spirv_builtin.h in #1712
Add a JSON output for the ClangProgModelBuiltinEmitter.
This patch also include header generator for libclc that consumes the JSON ouput,
which gets automatically trigged on changes on the SPIRVBuiltins.td.
Signed-off-by: Victor Lomuller [email protected]