Skip to content
Snippets Groups Projects

vkd3d-shader/dxil: Fix two bugs in handling LLVM forward-referenced type/value pairs.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:llvm_pair_bugs into master
1 unresolved thread

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Giovanni Mascellani approved this merge request

    approved this merge request

    • -    cmp = sm6_parser_get_value_by_ref(sm6, record, type, &i);
      +    /* Load a type/value pair for a forward-referenced comparand. This corresponds
      +     * to the use of getValueTypePair() instead of popValue() in the LLVM source. */
      +    cmp = sm6_parser_get_value_by_ref(sm6, record, NULL, &i);

      That's not the main thing I'd like to know when reading the commit though. Why is passing "type" here the wrong thing to do? In what kind of cases does is break?

    • That's not the main thing I'd like to know when reading the commit though. Why is passing "type" here the wrong thing to do? In what kind of cases does is break?

      The word "obvious" is unfortunately more subjective than one would often like to think, but isn't that information rather "obvious", or at least simple to work out, when reading sm6_parser_get_value_idx_by_ref()? The comment even hints in that direction. Everybody has their own thought processes, of course, but reading that function makes it pretty evident to me that setting or not setting that parameter is going to change the number of DXIL operands that get consumed (if the reference is inedeed a forward reference), so it's quite likely that either it must be set or it must not.

      We could certainly decide that in general our code is less commented than it should be (and to be honest sometimes I'm tempted to start working on patches that do nothing but add comments to code that already exists), but honestly I wouldn't mention this as a glaring example of that. That said, it's certainly legitimate to ask for more clarity.

    • This is a case of, "because the LLVM developers say so." It's not fully documented, therefore the source code is the documentation. I expanded the comment.

    • Please register or sign in to reply
  • Giovanni Mascellani mentioned in merge request !1102 (closed)

    mentioned in merge request !1102 (closed)

  • Conor McCarthy added 2 commits

    added 2 commits

    • e064f0ae - vkd3d-shader/dxil: Load a type/value pair for a forward-referenced comparand...
    • 85b71907 - vkd3d-shader/dxil: Load a type/value pair for a forward-referenced source in...

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Henri Verbeet added 16 commits

    added 16 commits

    • 85b71907...95bd06be - 14 commits from branch wine:master
    • 6a86e1a8 - vkd3d-shader/dxil: Load forward-referenced comparands as value/type pairs in...
    • 2bf51778 - vkd3d-shader/dxil: Load forward-referenced sources as value/type pairs in sm6_parser_emit_store().

    Compare with previous version

Please register or sign in to reply
Loading