[ORC] Add signext on @sum() arguments in test. #113308
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Ping! |
| std::string I32RetExt = ""; | ||
| std::string I32ArgExt = ""; |
There was a problem hiding this comment.
These can just be StringRefs.
|
|
||
| std::ostringstream OS; | ||
| OS << "define " << I32RetExt << " i32 " | ||
| << R"(@sum()" << "i32 " << I32ArgExt << "%x, i32 " << I32ArgExt << "%y)" |
There was a problem hiding this comment.
I think R"(@sum()" could just be "@sum(".
| if (StringRef(TargetTriple).starts_with("s390x-ibm-linux")) | ||
| I32RetExt = I32ArgExt = "signext "; |
There was a problem hiding this comment.
Is there a way that we could move this out into a common header location that could be used elsewhere in LLVM, or at least in the tests?
E.g.
StringRef getExtension(const Triple &TT) {
switch (TT.getArch()) {
case Triple::s390x:
if (TT.getVendor() == Triple::IBM && TT.getOS() == Triple::Linux)
return "signext ";
break;
default:
break;
}
return "";
}I'm guessing that we'll need this for Kaleidoscope and some other examples and tests too?
There was a problem hiding this comment.
Just re-read your comment -- I think a common utility like the above could be handy, but doesn't apply in this C API test where we don't have an llvm::Triple available.
ok - I reworked the patch a little so that TargetLibraryInfo is queried instead after constructing a Triple based on the string. Also put this into a little struct that might be reused in other places if needed, as you mentioned. |
|
ping - is the new version like something you were thinking of? |
Make sure the inlined @sum() function has the right extension attributes on its arguments. A new struct TargetI32ArgExtensions is added that sets the Ret/Arg extension strings given a string TargetTriple. This might be used elsewhere as well for this purpose if needed. Fixes: llvm#112503
Use static std::strings for the textual representations of @sum(). The usage of static variables is already there, e.g. with TargetTriple, so I have assumed that this is thread-safe.
The TargetTriple is not the full Triple class, but just the string representation. If the complete version of this is needed, the full Triple is needed so that it can be passed to TargetLibraryInfo getExtAttrForI32Param/Return. For now, there is just a string recognition of s390 which does need the extensions. Maybe this is ok for now as this is just a test, after all..?
Fixes: #112503