Shader_IR: Address Feedback.

src/common/hash.h

@@ -72,7 +72,7 @@ struct HashableStruct {
 
 struct PairHash {
     template <class T1, class T2>
-    std::size_t operator()(const std::pair<T1, T2>& pair) const {
+    std::size_t operator()(const std::pair<T1, T2>& pair) const noexcept {
         std::size_t seed = std::hash<T1>()(pair.first);
         boost::hash_combine(seed, std::hash<T2>()(pair.second));
         return seed;

src/video_core/engines/const_buffer_engine_interface.h

@@ -34,6 +34,10 @@ struct SamplerDescriptor {
         return raw == rhs.raw;
     }
 
+    bool operator!=(const SamplerDescriptor& rhs) const noexcept {
+        return !operator==(rhs);
+    }
+
     static SamplerDescriptor FromTicTexture(Tegra::Texture::TextureType tic_texture_type) {
         SamplerDescriptor result;
         switch (tic_texture_type) {
@@ -73,13 +77,12 @@ struct SamplerDescriptor {
             result.is_buffer.Assign(0);
             result.is_shadow.Assign(0);
             return result;
-        case Tegra::Texture::TextureType::Texture1DBuffer: {
+        case Tegra::Texture::TextureType::Texture1DBuffer:
             result.texture_type.Assign(Tegra::Shader::TextureType::Texture1D);
             result.is_array.Assign(0);
             result.is_buffer.Assign(1);
             result.is_shadow.Assign(0);
             return result;
-        }
         case Tegra::Texture::TextureType::Texture2DNoMipmap:
             result.texture_type.Assign(Tegra::Shader::TextureType::Texture2D);
             result.is_array.Assign(0);
@@ -105,7 +108,7 @@ static_assert(std::is_trivially_copyable_v<SamplerDescriptor>);
 
 class ConstBufferEngineInterface {
 public:
-    virtual ~ConstBufferEngineInterface() {}
+    virtual ~ConstBufferEngineInterface() = default;
     virtual u32 AccessConstBuffer32(ShaderType stage, u64 const_buffer, u64 offset) const = 0;
     virtual SamplerDescriptor AccessBoundSampler(ShaderType stage, u64 offset) const = 0;
     virtual SamplerDescriptor AccessBindlessSampler(ShaderType stage, u64 const_buffer,

src/video_core/shader/const_buffer_locker.cpp

@@ -22,6 +22,8 @@ ConstBufferLocker::ConstBufferLocker(Tegra::Engines::ShaderType shader_stage,
                                      Tegra::Engines::ConstBufferEngineInterface& engine)
     : stage{shader_stage}, engine{&engine} {}
 
+ConstBufferLocker::~ConstBufferLocker() = default;
+
 std::optional<u32> ConstBufferLocker::ObtainKey(u32 buffer, u32 offset) {
     const std::pair<u32, u32> key = {buffer, offset};
     const auto iter = keys.find(key);
@@ -29,7 +31,7 @@ std::optional<u32> ConstBufferLocker::ObtainKey(u32 buffer, u32 offset) {
         return iter->second;
     }
     if (!engine) {
-        return {};
+        return std::nullopt;
     }
     const u32 value = engine->AccessConstBuffer32(stage, buffer, offset);
     keys.emplace(key, value);
@@ -43,7 +45,7 @@ std::optional<SamplerDescriptor> ConstBufferLocker::ObtainBoundSampler(u32 offse
         return iter->second;
     }
     if (!engine) {
-        return {};
+        return std::nullopt;
     }
     const SamplerDescriptor value = engine->AccessBoundSampler(stage, offset);
     bound_samplers.emplace(key, value);
@@ -58,7 +60,7 @@ std::optional<Tegra::Engines::SamplerDescriptor> ConstBufferLocker::ObtainBindle
         return iter->second;
     }
     if (!engine) {
-        return {};
+        return std::nullopt;
     }
     const SamplerDescriptor value = engine->AccessBindlessSampler(stage, buffer, offset);
     bindless_samplers.emplace(key, value);

src/video_core/shader/const_buffer_locker.h

@@ -16,6 +16,11 @@ using BoundSamplerMap = std::unordered_map<u32, Tegra::Engines::SamplerDescripto
 using BindlessSamplerMap =
     std::unordered_map<std::pair<u32, u32>, Tegra::Engines::SamplerDescriptor, Common::PairHash>;
 
+/**
+ * The ConstBufferLocker is a class use to interface the 3D and compute engines with the shader
+ * compiler. with it, the shader can obtain required data from GPU state and store it for disk
+ * shader compilation.
+ **/
 class ConstBufferLocker {
 public:
     explicit ConstBufferLocker(Tegra::Engines::ShaderType shader_stage);
@@ -23,6 +28,8 @@ public:
     explicit ConstBufferLocker(Tegra::Engines::ShaderType shader_stage,
                                Tegra::Engines::ConstBufferEngineInterface& engine);
 
+    ~ConstBufferLocker();
+
     /// Retrieves a key from the locker, if it's registered, it will give the registered value, if
     /// not it will obtain it from maxwell3d and register it.
     std::optional<u32> ObtainKey(u32 buffer, u32 offset);

src/video_core/shader/control_flow.cpp

@@ -41,14 +41,10 @@ BlockBranchInfo MakeBranchInfo(Args&&... args) {
     return std::make_shared<BranchData>(T(std::forward<Args>(args)...));
 }
 
-bool BlockBranchInfoAreEqual(BlockBranchInfo first, BlockBranchInfo second) {
-    return false; //(*first) == (*second);
-}
-
 bool BlockBranchIsIgnored(BlockBranchInfo first) {
     bool ignore = false;
     if (std::holds_alternative<SingleBranch>(*first)) {
-        auto branch = std::get_if<SingleBranch>(first.get());
+        const auto branch = std::get_if<SingleBranch>(first.get());
         ignore = branch->ignore;
     }
     return ignore;
@@ -151,10 +147,10 @@ std::optional<BranchIndirectInfo> TrackBranchIndirectInfo(const CFGRebuildState&
     const Instruction instr = {state.program_code[pos]};
     const auto opcode = OpCode::Decode(instr);
     if (opcode->get().GetId() != OpCode::Id::BRX) {
-        return {};
+        return std::nullopt;
     }
     if (instr.brx.constant_buffer != 0) {
-        return {};
+        return std::nullopt;
     }
     track_register = instr.gpr8.Value();
     result.relative_position = instr.brx.GetBranchExtend();
@@ -172,8 +168,8 @@ std::optional<BranchIndirectInfo> TrackBranchIndirectInfo(const CFGRebuildState&
         if (opcode->get().GetId() == OpCode::Id::LD_C) {
             if (instr.gpr0.Value() == track_register &&
                 instr.ld_c.type.Value() == Tegra::Shader::UniformType::Single) {
-                result.buffer = instr.cbuf36.index;
+                result.buffer = instr.cbuf36.index.Value();
-                result.offset = instr.cbuf36.GetOffset();
+                result.offset = static_cast<u32>(instr.cbuf36.GetOffset());
                 track_register = instr.gpr8.Value();
                 pos--;
                 found_track = true;
@@ -184,7 +180,7 @@ std::optional<BranchIndirectInfo> TrackBranchIndirectInfo(const CFGRebuildState&
     }
 
     if (!found_track) {
-        return {};
+        return std::nullopt;
     }
     found_track = false;
 
@@ -194,7 +190,7 @@ std::optional<BranchIndirectInfo> TrackBranchIndirectInfo(const CFGRebuildState&
             pos--;
             continue;
         }
-        const Instruction instr = {state.program_code[pos]};
+        const Instruction instr = state.program_code[pos];
         const auto opcode = OpCode::Decode(instr);
         if (opcode->get().GetId() == OpCode::Id::SHL_IMM) {
             if (instr.gpr0.Value() == track_register) {
@@ -208,7 +204,7 @@ std::optional<BranchIndirectInfo> TrackBranchIndirectInfo(const CFGRebuildState&
     }
 
     if (!found_track) {
-        return {};
+        return std::nullopt;
     }
     found_track = false;
 
@@ -218,7 +214,7 @@ std::optional<BranchIndirectInfo> TrackBranchIndirectInfo(const CFGRebuildState&
             pos--;
             continue;
         }
-        const Instruction instr = {state.program_code[pos]};
+        const Instruction instr = state.program_code[pos];
         const auto opcode = OpCode::Decode(instr);
         if (opcode->get().GetId() == OpCode::Id::IMNMX_IMM) {
             if (instr.gpr0.Value() == track_register) {
@@ -233,9 +229,9 @@ std::optional<BranchIndirectInfo> TrackBranchIndirectInfo(const CFGRebuildState&
     }
 
     if (!found_track) {
-        return {};
+        return std::nullopt;
     }
-    return {result};
+    return result;
 }
 
 std::pair<ParseResult, ParseInfo> ParseCode(CFGRebuildState& state, u32 address) {
@@ -440,8 +436,8 @@ std::pair<ParseResult, ParseInfo> ParseCode(CFGRebuildState& state, u32 address)
                     branches.emplace_back(value, target);
                 }
                 parse_info.end_address = offset;
-                parse_info.branch_info =
+                parse_info.branch_info = MakeBranchInfo<MultiBranch>(
-                    MakeBranchInfo<MultiBranch>(static_cast<u32>(instr.gpr8.Value()), branches);
+                    static_cast<u32>(instr.gpr8.Value()), std::move(branches));
 
                 return {ParseResult::ControlCaught, parse_info};
             } else {
@@ -486,7 +482,7 @@ bool TryInspectAddress(CFGRebuildState& state) {
         current_block.end = address - 1;
         new_block.branch = current_block.branch;
         BlockBranchInfo forward_branch = MakeBranchInfo<SingleBranch>();
-        auto branch = std::get_if<SingleBranch>(forward_branch.get());
+        const auto branch = std::get_if<SingleBranch>(forward_branch.get());
         branch->address = address;
         branch->ignore = true;
         current_block.branch = forward_branch;
@@ -504,7 +500,7 @@ bool TryInspectAddress(CFGRebuildState& state) {
     BlockInfo& block_info = CreateBlockInfo(state, address, parse_info.end_address);
     block_info.branch = parse_info.branch_info;
     if (std::holds_alternative<SingleBranch>(*block_info.branch)) {
-        auto branch = std::get_if<SingleBranch>(block_info.branch.get());
+        const auto branch = std::get_if<SingleBranch>(block_info.branch.get());
         if (branch->condition.IsUnconditional()) {
             return true;
         }
@@ -550,7 +546,7 @@ bool TryQuery(CFGRebuildState& state) {
     gather_labels(q2.ssy_stack, state.ssy_labels, block);
     gather_labels(q2.pbk_stack, state.pbk_labels, block);
     if (std::holds_alternative<SingleBranch>(*block.branch)) {
-        auto branch = std::get_if<SingleBranch>(block.branch.get());
+        const auto branch = std::get_if<SingleBranch>(block.branch.get());
         if (!branch->condition.IsUnconditional()) {
             q2.address = block.end + 1;
             state.queries.push_back(q2);
@@ -573,8 +569,8 @@ bool TryQuery(CFGRebuildState& state) {
         state.queries.push_back(std::move(conditional_query));
         return true;
     }
-    auto multi_branch = std::get_if<MultiBranch>(block.branch.get());
+    const auto multi_branch = std::get_if<MultiBranch>(block.branch.get());
-    for (auto& branch_case : multi_branch->branches) {
+    for (const auto& branch_case : multi_branch->branches) {
         Query conditional_query{q2};
         conditional_query.address = branch_case.address;
         state.queries.push_back(std::move(conditional_query));
@@ -612,7 +608,7 @@ void InsertBranch(ASTManager& mm, const BlockBranchInfo& branch_info) {
         return MakeExpr<ExprBoolean>(true);
     });
     if (std::holds_alternative<SingleBranch>(*branch_info)) {
-        auto branch = std::get_if<SingleBranch>(branch_info.get());
+        const auto branch = std::get_if<SingleBranch>(branch_info.get());
         if (branch->address < 0) {
             if (branch->kill) {
                 mm.InsertReturn(get_expr(branch->condition), true);
@@ -624,8 +620,8 @@ void InsertBranch(ASTManager& mm, const BlockBranchInfo& branch_info) {
         mm.InsertGoto(get_expr(branch->condition), branch->address);
         return;
     }
-    auto multi_branch = std::get_if<MultiBranch>(branch_info.get());
+    const auto multi_branch = std::get_if<MultiBranch>(branch_info.get());
-    for (auto& branch_case : multi_branch->branches) {
+    for (const auto& branch_case : multi_branch->branches) {
         mm.InsertGoto(MakeExpr<ExprGprEqual>(multi_branch->gpr, branch_case.cmp_value),
                       branch_case.address);
     }

src/video_core/shader/control_flow.h

@@ -51,6 +51,10 @@ public:
                std::tie(b.condition, b.address, b.kill, b.is_sync, b.is_brk, b.ignore);
     }
 
+    bool operator!=(const SingleBranch& b) const {
+        return !operator==(b);
+    }
+
     Condition condition{};
     s32 address{exit_branch};
     bool kill{};
@@ -67,7 +71,7 @@ struct CaseBranch {
 
 class MultiBranch {
 public:
-    MultiBranch(u32 gpr, std::vector<CaseBranch>& branches)
+    MultiBranch(u32 gpr, std::vector<CaseBranch>&& branches)
         : gpr{gpr}, branches{std::move(branches)} {}
 
     u32 gpr{};

src/video_core/shader/decode.cpp

@@ -155,7 +155,7 @@ void ShaderIR::Decode() {
         [[fallthrough]];
     case CompileDepth::BruteForce: {
         coverage_begin = main_offset;
-        const u32 shader_end = program_code.size();
+        const std::size_t shader_end = program_code.size();
         coverage_end = shader_end;
         for (u32 label = main_offset; label < shader_end; label++) {
             basic_blocks.insert({label, DecodeRange(label, label + 1)});

src/video_core/shader/decode/texture.cpp

@@ -284,7 +284,7 @@ u32 ShaderIR::DecodeTexture(NodeBlock& bb, u32 pc) {
 
 const Sampler& ShaderIR::GetSampler(const Tegra::Shader::Sampler& sampler,
                                     std::optional<SamplerInfo> sampler_info) {
-    const auto offset = static_cast<std::size_t>(sampler.index.Value());
+    const auto offset = static_cast<u32>(sampler.index.Value());
 
     Tegra::Shader::TextureType type;
     bool is_array;
@@ -293,17 +293,14 @@ const Sampler& ShaderIR::GetSampler(const Tegra::Shader::Sampler& sampler,
         type = sampler_info->type;
         is_array = sampler_info->is_array;
         is_shadow = sampler_info->is_shadow;
+    } else if (auto sampler = locker.ObtainBoundSampler(offset); sampler) {
+        type = sampler->texture_type.Value();
+        is_array = sampler->is_array.Value() != 0;
+        is_shadow = sampler->is_shadow.Value() != 0;
     } else {
-        auto sampler = locker.ObtainBoundSampler(offset);
+        type = Tegra::Shader::TextureType::Texture2D;
-        if (sampler) {
+        is_array = false;
-            type = sampler->texture_type.Value();
+        is_shadow = false;
-            is_array = sampler->is_array.Value() != 0;
-            is_shadow = sampler->is_shadow.Value() != 0;
-        } else {
-            type = Tegra::Shader::TextureType::Texture2D;
-            is_array = false;
-            is_shadow = false;
-        }
     }
 
     // If this sampler has already been used, return the existing mapping.
@@ -320,7 +317,7 @@ const Sampler& ShaderIR::GetSampler(const Tegra::Shader::Sampler& sampler,
     const std::size_t next_index = used_samplers.size();
     const Sampler entry{offset, next_index, type, is_array, is_shadow};
     return *used_samplers.emplace(entry).first;
-}
+} // namespace VideoCommon::Shader
 
 const Sampler& ShaderIR::GetBindlessSampler(const Tegra::Shader::Register& reg,
                                             std::optional<SamplerInfo> sampler_info) {
@@ -336,17 +333,14 @@ const Sampler& ShaderIR::GetBindlessSampler(const Tegra::Shader::Register& reg,
         type = sampler_info->type;
         is_array = sampler_info->is_array;
         is_shadow = sampler_info->is_shadow;
+    } else if (auto sampler = locker.ObtainBindlessSampler(cbuf_index, cbuf_offset); sampler) {
+        type = sampler->texture_type.Value();
+        is_array = sampler->is_array.Value() != 0;
+        is_shadow = sampler->is_shadow.Value() != 0;
     } else {
-        auto sampler = locker.ObtainBindlessSampler(cbuf_index, cbuf_offset);
+        type = Tegra::Shader::TextureType::Texture2D;
-        if (sampler) {
+        is_array = false;
-            type = sampler->texture_type.Value();
+        is_shadow = false;
-            is_array = sampler->is_array.Value() != 0;
-            is_shadow = sampler->is_shadow.Value() != 0;
-        } else {
-            type = Tegra::Shader::TextureType::Texture2D;
-            is_array = false;
-            is_shadow = false;
-        }
     }
 
     // If this sampler has already been used, return the existing mapping.

src/video_core/shader/expr.h

@@ -127,6 +127,10 @@ public:
         return gpr == b.gpr && value == b.value;
     }
 
+    bool operator!=(const ExprGprEqual& b) const {
+        return !operator==(b);
+    }
+
     u32 gpr;
     u32 value;
 };