Subzero: fix Call on bool-returning functions
On some ABIs, C++ functions that return bool only set the least
significant bits of the return register (e.g. AL on x64). The Call
implementation in SubzeroReactor would use uninitialized i32 target
to hold the result of a bool-returning function, but this would return
erroneous results when the called function would return false by setting
the LSB to 0, but with a non-zero MSB, thus returning true.
This change makes sure to truncate the i32 result to i8 (bool),
returning true only if the lsb is non-zero.
Bug: b/151158858
Change-Id: I41055a94d7f8045da503f27881ed887c1926f77b
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/42068
Tested-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: Nicolas Capens <nicolascapens@google.com>
diff --git a/src/Reactor/ReactorUnitTests.cpp b/src/Reactor/ReactorUnitTests.cpp
index 5eabcb6..bcfd13d 100644
--- a/src/Reactor/ReactorUnitTests.cpp
+++ b/src/Reactor/ReactorUnitTests.cpp
@@ -1667,6 +1667,35 @@
EXPECT_EQ(c.str, "hello world");
}
+TEST(ReactorUnitTests, CallBoolReturnFunction)
+{
+ struct Class
+ {
+ static bool IsEven(int a)
+ {
+ return a % 2 == 0;
+ }
+ };
+
+ FunctionT<int(int)> function;
+ {
+ Int a = function.Arg<0>();
+ Bool res = Call(Class::IsEven, a);
+ If(res)
+ {
+ Return(1);
+ }
+ Return(0);
+ }
+
+ auto routine = function("one");
+
+ for(int i = 0; i < 10; ++i)
+ {
+ EXPECT_EQ(routine(i), i % 2 == 0);
+ }
+}
+
TEST(ReactorUnitTests, Call_Args4)
{
struct Class
diff --git a/src/Reactor/SubzeroReactor.cpp b/src/Reactor/SubzeroReactor.cpp
index 7196c79..6602ddd 100644
--- a/src/Reactor/SubzeroReactor.cpp
+++ b/src/Reactor/SubzeroReactor.cpp
@@ -144,31 +144,50 @@
}
}
-// Wrapper for calls on C functions with Ice types
-Ice::Variable *Call(Ice::Cfg *function, Ice::CfgNode *basicBlock, Ice::Type retTy, void const *fptr, const std::vector<Ice::Operand *> &iceArgs, bool isVariadic)
-{
- // Subzero doesn't support boolean return values. Replace with an i32.
- if(retTy == Ice::IceType_i1)
- {
- retTy = Ice::IceType_i32;
- }
+// TODO(amaiorano): remove this prototype once these are moved to separate header/cpp
+Ice::Variable *createTruncate(Ice::Cfg *function, Ice::CfgNode *basicBlock, Ice::Operand *from, Ice::Type toType);
+// Wrapper for calls on C functions with Ice types
+Ice::Variable *Call(Ice::Cfg *function, Ice::CfgNode *basicBlock, Ice::Type retTy, Ice::Operand *callTarget, const std::vector<Ice::Operand *> &iceArgs, bool isVariadic)
+{
Ice::Variable *ret = nullptr;
- if(retTy != Ice::IceType_void)
+
+ // Subzero doesn't support boolean return values. Replace with an i32 temporarily,
+ // then truncate result to bool.
+ // TODO(b/151158858): Add support to Subzero's InstCall for bool-returning functions
+ const bool returningBool = (retTy == Ice::IceType_i1);
+ if(returningBool)
+ {
+ ret = function->makeVariable(Ice::IceType_i32);
+ }
+ else if(retTy != Ice::IceType_void)
{
ret = function->makeVariable(retTy);
}
- auto call = Ice::InstCall::create(function, iceArgs.size(), ret, getConstantPointer(function->getContext(), fptr), false, false, isVariadic);
+ auto call = Ice::InstCall::create(function, iceArgs.size(), ret, callTarget, false, false, isVariadic);
for(auto arg : iceArgs)
{
call->addArg(arg);
}
basicBlock->appendInst(call);
+
+ if(returningBool)
+ {
+ // Truncate result to bool so that if any (lsb) bits were set, result will be true
+ ret = createTruncate(function, basicBlock, ret, Ice::IceType_i1);
+ }
+
return ret;
}
+Ice::Variable *Call(Ice::Cfg *function, Ice::CfgNode *basicBlock, Ice::Type retTy, void const *fptr, const std::vector<Ice::Operand *> &iceArgs, bool isVariadic)
+{
+ Ice::Operand *callTarget = getConstantPointer(function->getContext(), fptr);
+ return Call(function, basicBlock, retTy, callTarget, iceArgs, isVariadic);
+}
+
// Wrapper for calls on C functions with Ice types
template<typename Return, typename... CArgs, typename... RArgs>
Ice::Variable *Call(Ice::Cfg *function, Ice::CfgNode *basicBlock, Return(fptr)(CArgs...), RArgs &&... args)
@@ -187,6 +206,14 @@
return result;
}
+Ice::Variable *createTruncate(Ice::Cfg *function, Ice::CfgNode *basicBlock, Ice::Operand *from, Ice::Type toType)
+{
+ Ice::Variable *to = function->makeVariable(toType);
+ Ice::InstCast *cast = Ice::InstCast::create(function, Ice::InstCast::Trunc, to, from);
+ basicBlock->appendInst(cast);
+ return to;
+}
+
Ice::Variable *createLoad(Ice::Cfg *function, Ice::CfgNode *basicBlock, Ice::Operand *ptr, Ice::Type type, unsigned int align)
{
// TODO(b/148272103): InstLoad assumes that a constant ptr is an offset, rather than an
@@ -4138,18 +4165,7 @@
Value *Call(RValue<Pointer<Byte>> fptr, Type *retTy, std::initializer_list<Value *> args, std::initializer_list<Type *> argTys)
{
RR_DEBUG_INFO_UPDATE_LOC();
- Ice::Variable *ret = nullptr;
- if(retTy != nullptr)
- {
- ret = ::function->makeVariable(T(retTy));
- }
- auto call = Ice::InstCall::create(::function, args.size(), ret, V(fptr.value), false);
- for(auto arg : args)
- {
- call->addArg(V(arg));
- }
- ::basicBlock->appendInst(call);
- return V(ret);
+ return V(sz::Call(::function, ::basicBlock, T(retTy), V(fptr.value), V(args), false));
}
void Breakpoint()
