Add comparison rule, tolerance if applicable, and hex64 view to test failure text output

[bob80905]

This PR implements some extra quality of life to address confusion in seemingly identical actual and expected results.
Fixes #289

Here's some sample output of what I see when I force a test to fail, manually changing expected values.

[spall]

It might be better to use the c++ hex float format when printing float values since I don't think the standard hex values will be properly interpreted if included in a test file.

[llvm-beanz]

It might be better to use the c++ hex float format when printing float values since I don't think the standard hex values will be properly interpreted if included in a test file.

Should we maybe do hex float for the epsilon and ULP compares, but hex integer for the exact match? I feel like the exact match is more likely to be used with integer data than float data.

[alsepkow]

Minor nit: Would it make sense to move the declarations of Str and OS to the beginning of this function and get rid of TestRuleStr and TestRuleOStr? Would probably want to increase the size of the llvm::SmallString in that case.

[alsepkow]

Some minor comments. Available to re-review when addressed.

[alsepkow]

Up to you, but I think it would be cleaner to use templates for this instead of a macro.
I asked co-pilot to re-write using templates.

Also includes a simplification of the logic of the for loop formatting the output string.

Edit: After writing this comment I see the same pattern used elsewhere in Pipeline.cpp. The templated version still seems cleaner IMO. And I do see the same pattern with ENUM_CASE in pipeline.h as well, but all of those cases have very simple logic in the macro. I'll still leave the decision up to you.

Something like this:

template <typename T>
std::string formatBuffer(offloadtest::Buffer* B, offloadtest::Rule rule) {
  llvm::MutableArrayRef<T> arr(reinterpret_cast<T*>(B->Data.get()), B->Size / sizeof(T));
  if (arr.empty()) return "";

  std::string result = "[ " + bitPatternAsHex64(arr[0], rule);
  for (size_t i = 1; i < arr.size(); ++i)
    result += ", " + bitPatternAsHex64(arr[i], rule);
  result += " ]";
  return result;
}

static const std::string getBufferStr(offloadtest::Buffer* B, offloadtest::Rule rule) {
  using DF = offloadtest::DataFormat;
  switch (B->Format) {
    case DF::Hex8:    return formatBuffer<llvm::yaml::Hex8>(B, rule);
    case DF::Hex16:   return formatBuffer<llvm::yaml::Hex16>(B, rule);
    case DF::Hex32:   return formatBuffer<llvm::yaml::Hex32>(B, rule);
    case DF::Hex64:   return formatBuffer<llvm::yaml::Hex64>(B, rule);
    case DF::UInt16:  return formatBuffer<uint16_t>(B, rule);
    case DF::UInt32:  return formatBuffer<uint32_t>(B, rule);
    case DF::UInt64:  return formatBuffer<uint64_t>(B, rule);
    case DF::Int16:   return formatBuffer<int16_t>(B, rule);
    case DF::Int32:   return formatBuffer<int32_t>(B, rule);
    case DF::Int64:   return formatBuffer<int64_t>(B, rule);
    case DF::Float16: return formatBuffer<llvm::yaml::Hex16>(B, rule); // assuming no native float16
    case DF::Float32: return formatBuffer<float>(B, rule);
    case DF::Float64: return formatBuffer<double>(B, rule);
    case DF::Bool:    return formatBuffer<uint32_t>(B, rule); // Because sizeof(bool) is 1 but HLSL represents a bool using 4 bytes.
    default:          return "UHO SCOOBY";
  }
}

[inbelic]

I would +1 on the templated version fwiw

[bob80905]

Changed Pipeline.cpp and Check.cpp, I think I should leave pipeline.h as is.

[alsepkow]

Do you think it would be helpful to have a default string with something like "getBufferStr: Unrecognized DataFormat" to make it quicker to debug in the future if a format type was added and we forgot to handle it?

[alsepkow]

Reviewing together with @inbelic he reminded me that llvm defaults to ensure all cases are covered. So I think NOT having a default is actually preferred?

[bob80905]

I'll leave it without a default for now.

[alsepkow]

Could we use llvm::raw_svector_ostream to avoid the include of ? Or is not able to handle the conversion of std::hex?

[bob80905]

The std library is the only thing aware of std::hex, unfortunately none of the other llvm ostreams are compatible with std::hex / hexfloat.

[inbelic]

nit: Do we want to add a test case of the correct error message output?

We could use FileCheck to ensure the hex values or whatnot are being formatted as expected

[alsepkow]

LGTM. But I think you should wait for sign off from llvm-beanz

[llvm-beanz]

Can we prefix these with 0x so that we know they're hex?