Fix lowering and optimization of 64-bit absolute addresses
x86-64 does not support 64-bit immediates as absolute memory addresses.
They have to be stored in a register, which can then be used as [base].
Previously we addressed this at the SubzeroReactor level by emitting a
Bitcast from an Ice::Operand to an Ice::Variable, for which Subzero
already supported 64-bit constants as input.
This change implements X86OperandMem creation from a 64-bit constant
operand by letting legalize() move it into a GPR and using it as the
memory operand's base register.
A Reactor unit test is added to exercise this.
Another issue was that doLoadOpt() assumed all load instructions are
candidates for fusing into a subsequent instruction which takes the
result of the load. This isn't true when for 64-bit constant addresses
an instruction to copy it into a register is inserted.
For now this case is simply skipped. A future optimization could adjust
the iterators properly so the load from [base] can be fused with the
next instruction.
Lastly, it is possible for a 64-bit constant to fit within a 32-bit
immediate, in which case legalize() by default does not perform the copy
into a GPR (note this is to allow moves and calls with 64-bit
immediates, where they are legal), and simply returns the 64-bit
constant. So we must not allow legalization to an immediate in this
case. Note that while we could replace it with a 32-bit constant, it's
rare for absolute addresses to fit in this range, and it would be
non-deterministic which path is taken, so for consistency we don't
perform this optimization.
Bug: b/148272103
Change-Id: I5fcfa971dc93f2307202ee11619e84c65fe46188
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/52768
Tested-by: Nicolas Capens <nicolascapens@google.com>
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
diff --git a/src/Reactor/Optimizer.cpp b/src/Reactor/Optimizer.cpp
index 8be37b5..d5f95e8 100644
--- a/src/Reactor/Optimizer.cpp
+++ b/src/Reactor/Optimizer.cpp
@@ -319,14 +319,6 @@
continue;
}
- // TODO(b/148272103): InstLoad assumes that a constant ptr is an offset, rather than an
- // absolute address. We need to make sure we don't replace a variable with a constant
- // on this load.
- if(llvm::isa<Ice::Constant>(storeValue))
- {
- continue;
- }
-
replace(load, storeValue);
for(size_t i = 0; i < addressUses.loads.size(); i++)
@@ -490,15 +482,6 @@
continue;
}
- // TODO(b/148272103): InstLoad assumes that a constant ptr is an offset, rather than an
- // absolute address. We need to make sure we don't replace a variable with a constant
- // on this load.
- if(llvm::isa<Ice::Constant>(storeValue))
- {
- unmatchedLoads = true;
- continue;
- }
-
replace(inst, storeValue);
}
}
diff --git a/src/Reactor/SubzeroReactor.cpp b/src/Reactor/SubzeroReactor.cpp
index 41c07c8..53377ca 100644
--- a/src/Reactor/SubzeroReactor.cpp
+++ b/src/Reactor/SubzeroReactor.cpp
@@ -174,15 +174,6 @@
return Call(function, basicBlock, retTy, reinterpret_cast<void const *>(fptr), iceArgs, false);
}
-// Returns a non-const variable copy of const v
-Ice::Variable *createUnconstCast(Ice::Cfg *function, Ice::CfgNode *basicBlock, Ice::Constant *v)
-{
- Ice::Variable *result = function->makeVariable(v->getType());
- Ice::InstCast *cast = Ice::InstCast::create(function, Ice::InstCast::Bitcast, result, v);
- basicBlock->appendInst(cast);
- return result;
-}
-
Ice::Variable *createTruncate(Ice::Cfg *function, Ice::CfgNode *basicBlock, Ice::Operand *from, Ice::Type toType)
{
Ice::Variable *to = function->makeVariable(toType);
@@ -193,14 +184,6 @@
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
- // absolute address. We circumvent this by casting to a non-const variable, and loading
- // from that.
- if(auto *cptr = llvm::dyn_cast<Ice::Constant>(ptr))
- {
- ptr = sz::createUnconstCast(function, basicBlock, cptr);
- }
-
Ice::Variable *result = function->makeVariable(type);
auto load = Ice::InstLoad::create(function, result, ptr, align);
basicBlock->appendInst(load);
diff --git a/tests/ReactorUnitTests/ReactorUnitTests.cpp b/tests/ReactorUnitTests/ReactorUnitTests.cpp
index b6c0d9cf..6b85a48 100644
--- a/tests/ReactorUnitTests/ReactorUnitTests.cpp
+++ b/tests/ReactorUnitTests/ReactorUnitTests.cpp
@@ -376,6 +376,23 @@
EXPECT_EQ(result, 7);
}
+TEST(ReactorUnitTests, ConstantPointer)
+{
+ int c = 44;
+
+ FunctionT<int()> function;
+ {
+ Int x = *Pointer<Int>(ConstantPointer(&c));
+
+ Return(x);
+ }
+
+ auto routine = function(testName().c_str());
+
+ int result = routine();
+ EXPECT_EQ(result, 44);
+}
+
// This test excercises the Optimizer::eliminateLoadsFollowingSingleStore() optimization pass.
// The three load operations for `y` should get eliminated.
// TODO(b/180665600): Check that the optimization took place.
diff --git a/third_party/subzero/src/IceTargetLoweringX86BaseImpl.h b/third_party/subzero/src/IceTargetLoweringX86BaseImpl.h
index d4c41d3..37ab794 100644
--- a/third_party/subzero/src/IceTargetLoweringX86BaseImpl.h
+++ b/third_party/subzero/src/IceTargetLoweringX86BaseImpl.h
@@ -823,11 +823,15 @@
// Determine whether the current instruction is a Load instruction or
// equivalent.
if (auto *Load = llvm::dyn_cast<InstLoad>(CurInst)) {
- // An InstLoad always qualifies.
- LoadDest = Load->getDest();
- constexpr bool DoLegalize = false;
- LoadSrc = formMemoryOperand(Load->getLoadAddress(), LoadDest->getType(),
- DoLegalize);
+ // An InstLoad qualifies unless it uses a 64-bit absolute address,
+ // which requires legalization to insert a copy to register.
+ // TODO(b/148272103): Fold these after legalization.
+ if (!Traits::Is64Bit || !llvm::isa<Constant>(Load->getLoadAddress())) {
+ LoadDest = Load->getDest();
+ constexpr bool DoLegalize = false;
+ LoadSrc = formMemoryOperand(Load->getLoadAddress(),
+ LoadDest->getType(), DoLegalize);
+ }
} else if (auto *Intrin = llvm::dyn_cast<InstIntrinsic>(CurInst)) {
// An AtomicLoad intrinsic qualifies as long as it has a valid memory
// ordering, and can be implemented in a single instruction (i.e., not
@@ -8121,20 +8125,22 @@
auto *Offset = llvm::dyn_cast<Constant>(Opnd);
assert(Base || Offset);
if (Offset) {
- // During memory operand building, we do not blind or pool the constant
- // offset, we will work on the whole memory operand later as one entity
- // later, this save one instruction. By turning blinding and pooling off,
- // we guarantee legalize(Offset) will return a Constant*.
if (!llvm::isa<ConstantRelocatable>(Offset)) {
- Offset = llvm::cast<Constant>(legalize(Offset));
- }
+ if (llvm::isa<ConstantInteger64>(Offset)) {
+ // Memory operands cannot have 64-bit immediates, so they must be
+ // legalized into a register only.
+ Base = llvm::cast<Variable>(legalize(Offset, Legal_Reg));
+ Offset = nullptr;
+ } else {
+ Offset = llvm::cast<Constant>(legalize(Offset));
- assert(llvm::isa<ConstantInteger32>(Offset) ||
- llvm::isa<ConstantRelocatable>(Offset));
+ assert(llvm::isa<ConstantInteger32>(Offset) ||
+ llvm::isa<ConstantRelocatable>(Offset));
+ }
+ }
}
Mem = X86OperandMem::create(Func, Ty, Base, Offset);
}
- // Do legalization, which contains pooling or do pooling.
return llvm::cast<X86OperandMem>(DoLegalize ? legalize(Mem) : Mem);
}