[PATCH] D137815: [DirectX backend] Fix build and test error caused by out of sync with upstream change.

[Tex Riddell via Phabricator via llvm-commits]

tex3d added a comment.

I think we need to add MemoryEffects DXIL canonicalization to translate from attribute values with more nuance to conservative versions for DXIL serialization - see comment on `DXILPrepare.cpp`.  Otherwise, we could have duplicate attribute sets once these are translated in DXILBitcodeWriter, which would result in bitcode differences between equivalent DXIL IR.  In other words, if you round-trip the DXIL output from here, the bitcode would change.

We could use some unit tests for how we sanitize and translate different MemoryEffects combinations as well.



================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:57
                        Attribute::DereferenceableOrNull,
+                       Attribute::Memory,
                        Attribute::NoRedZone,
----------------
I think merely allowing this through is insufficient.  We should canonicalize the MemoryEffects attribute to a few conservative values for DXIL to avoid duplicate attribute sets once these are translated during serialization.


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp:942
+          } else {
+            if (ME.onlyReadsMemory()) {
+              Record.push_back(0);
----------------
If function only reads from arg mem, won't `ME.onlyReadsMemory()` be true, leading to both attributes: `readonly` and `argmemonly`?  I think in this case you only want `argmemonly`.

I think instead, we need something like:
```
if (ME.onlyAccessesArgPointees()) {
  ...
} else if (ME.onlyReadsMemory()) {
  ...
}
```


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp:948
+              Record.push_back(0);
+              Record.push_back(bitc::ATTR_KIND_ARGMEMONLY);
+            }
----------------
Should we have unit tests to ensure proper translation of different/interesting MemoryEffects combinations?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137815/new/

https://reviews.llvm.org/D137815