[PATCH] D122081: Add DXILPrepare CodeGen pass

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Seems reasonable



================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:48
+      Attribute::SafeStack, Attribute::StructRet, Attribute::SanitizeAddress,
+      Attribute::SanitizeThread, Attribute::SanitizeMemory, Attribute::UWTable,
+      Attribute::ZExt>(Attr);
----------------
It's amusing to consider the implications of sanitizer attributes on DXIL. :) (no action required)


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:79
+          I->replaceAllUsesWith(Builder.CreateFSub(Zero, In));
+          I->eraseFromParent();
+        }
----------------
If an `fneg` uses an `fneg`, there's a potential use-after-free here. The recommended best practice is to accumulate instructions and delete them after the loop to avoid modification during iteration. I *think* it's safe to do the insertion during iteration. There's also the early_inc iterator: https://reviews.llvm.org/D49956

Anyway, you've got options.


================
Comment at: llvm/lib/Target/DirectX/DXILPrepare.cpp:64
+  }
+#include "llvm/IR/Attributes.inc"
+      for (auto &BB : F) {
----------------
nikic wrote:
> beanz wrote:
> > nikic wrote:
> > > Do you really need Attributes.inc here? You should be able to loop over all attributes using Attribute::None and Attribute::EndAttrKind.
> > I don't strictly need it, but if `isValidForDXIL` gets called with a constant it should get constant-folded to true/false. That isn't the case if I call it in a loop.
> You can construct an AttributeMask with all the invalid attributes upfront, and then remove that mask, rather than removing each attribute individually. In that case, constant folding shouldn't matter, and you can probably drop D122079 entirely.
(This is already resolved, but in general, strong +1 to more data, less generated code.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122081