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

[Xiang Li via Phabricator via llvm-commits]

python3kgae marked 3 inline comments as done.
python3kgae added inline comments.


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:57
                        Attribute::DereferenceableOrNull,
+                       Attribute::Memory,
                        Attribute::NoRedZone,
----------------
tex3d wrote:
> 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.
Created https://github.com/llvm/llvm-project/issues/58950 to track this.


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp:942
+          } else {
+            if (ME.onlyReadsMemory()) {
+              Record.push_back(0);
----------------
tex3d wrote:
> tex3d wrote:
> > 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()) {
> >   ...
> > }
> > ```
> Then again, I do see some other case with both these attributes, so maybe this behavior is desirable.
It is OK for a function has both readnone argmemonly attribute.


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp:948
+              Record.push_back(0);
+              Record.push_back(bitc::ATTR_KIND_ARGMEMONLY);
+            }
----------------
tex3d wrote:
> Should we have unit tests to ensure proper translation of different/interesting MemoryEffects combinations?
We could always add tests.
But this PR is just supposed to fix the build and test, so no one is blocked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137815