Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid use of type manager in extact->construct folding #5684

Merged
merged 2 commits into from
May 31, 2024

Conversation

s-perron
Copy link
Collaborator

When dealing with structs the type manager merge two different structs
into a single entry if they have all of the same decorations and
element types. This is because they hash to the same value in the hash
table. This can cause problems if you need to get the id of a type from
the type manager because you could get either one. In this case, it
returns the wrong one.

The fix avoids using the type manager in one place. I have not
looked closely at other places the type manager is used to make
sure it is used safely everywhere.

Fixes #5624

When dealing with structs the type manager merge two different structs
into a single entry if they have all of the same decorations and
element types. This is because they hash to the same value in the hash
table. This can cause problems if you need to get the id of a type from
the type manager because you could get either one. In this case, it
returns the wrong one.

The fix avoids using the type manager in one place. I have not
looked closely at other places the type manager is used to make
sure it is used safely everywhere.

Fixes KhronosGroup#5624
@s-perron s-perron requested a review from Keenuts May 23, 2024 17:37
@LukasBanana
Copy link
Contributor

LukasBanana commented May 24, 2024

Hi, we ran into this issue yesterday and your fix helps us partially, but we had to make an amended to address the problem in our scenario:
We changed GetContainerType to GetContainerTypeId and return only the ID instead of the type instance, since you mentioned in #5624 that the type manager can get confused with isomorphic struct. CompositeInsertToCompositeConstruct will then use that ID for the call to BuildCompositeConstruct instead of using type_mgr->GetId(container_type) again:

uint32_t GetContainerTypeId(Instruction* inst) {
  assert(inst->opcode() == spv::Op::OpCompositeInsert);
  analysis::DefUseManager* def_use_manager = inst->context()->get_def_use_mgr();
  uint32_t container_type_id = GetElementType(
      inst->type_id(), inst->begin() + 4, inst->end() - 1, def_use_manager);
  return container_type_id;
}
...
bool CompositeInsertToCompositeConstruct(
    IRContext* context, Instruction* inst,
    const std::vector<const analysis::Constant*>&) {
  assert(inst->opcode() == spv::Op::OpCompositeInsert &&
         "Wrong opcode.  Should be OpCompositeInsert.");
  if (inst->NumInOperands() < 3) return false;

  analysis::TypeManager* type_mgr = context->get_type_mgr();
  std::map<uint32_t, uint32_t> values_inserted = GetInsertedValues(inst);
  const uint32_t container_type_id = GetContainerTypeId(inst);
  const analysis::Type* container_type = type_mgr->GetType(container_type_id);
  if (container_type == nullptr) {
    return false;
  }

  if (!DoInsertedValuesCoverEntireObject(container_type, values_inserted)) {
    return false;
  }

  Instruction* construct =
      BuildCompositeConstruct(container_type_id, values_inserted, inst);
  InsertConstructedObject(inst, construct);
  return true;
}

Please feel free to take this change if that works for you as it's usually a bit time consuming to come up with a smaller test case than just providing our large shaders.

UPDATE:
While my posted changes resolved an issue in our test case, there are still some other validation errors in our larger auto-generated shaders, so I can't fully confirm yet whether this one fixed the problem for us. Just as a heads-up that this should be taken with a grain of salt.

This removes a use of TypeManager::GetId by keeping the id around. This
avoid a potential problem if the type manager gets confused. These types
of bugs are hard to generate test cases for, so I do not have a test.
However, existing tests make sure that do not regress.
source/opt/folding_rules.cpp Show resolved Hide resolved
source/opt/folding_rules.cpp Show resolved Hide resolved
@s-perron s-perron requested a review from Keenuts May 30, 2024 19:32
@s-perron s-perron enabled auto-merge (squash) May 30, 2024 20:39
Copy link
Contributor

@Keenuts Keenuts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the details, I missed the part where the types could be replaced only partially, hence causing issues. LGTM!

@s-perron s-perron merged commit 148c97f into KhronosGroup:main May 31, 2024
24 checks passed
@s-perron s-perron deleted the i5624 branch May 31, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SPIRV code fails validation after optimization
4 participants