This is an archive of the discontinued LLVM Phabricator instance.

Add DXILPrepare CodeGen pass
ClosedPublic

Authored by beanz on Mar 19 2022, 1:26 PM.

Details

Summary

The DXIL Prepare pass handles the IR mutations required to convert
modern LLVM IR into something that more closely resembles LLVM-3.7 IR
so that the DXIL bitcode writer can emit 3.7 IR.

This change adds the codegen pass handling the first two IR
transformations:

  • stripping new function attributes
  • converting fneg into fsub

Diff Detail

Event Timeline

beanz created this revision.Mar 19 2022, 1:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2022, 1:26 PM
beanz requested review of this revision.Mar 19 2022, 1:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2022, 1:26 PM
nikic added a subscriber: nikic.Mar 19 2022, 2:08 PM
nikic added inline comments.
llvm/lib/Target/DirectX/DXILPrepare.cpp
65

Do you really need Attributes.inc here? You should be able to loop over all attributes using Attribute::None and Attribute::EndAttrKind.

76

Shouldn't this be negative zero?

beanz added inline comments.Mar 19 2022, 4:03 PM
llvm/lib/Target/DirectX/DXILPrepare.cpp
65

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.

76

Yep, you are 100% correct. Will update.

arsenm added a subscriber: arsenm.Mar 20 2022, 5:43 AM
arsenm added inline comments.
llvm/lib/Target/DirectX/DXILPrepare.cpp
77

Well this is a semantically wrong substitution

nikic added inline comments.Mar 20 2022, 6:27 AM
llvm/lib/Target/DirectX/DXILPrepare.cpp
65

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.

beanz updated this revision to Diff 417454.Mar 22 2022, 5:52 PM

Update to use an AttributeMask instead of Attributes.inc expansion, and correct fneg replacement.

rnk added a comment.Mar 22 2022, 7:03 PM

Seems reasonable

llvm/lib/Target/DirectX/DXILPrepare.cpp
48

It's amusing to consider the implications of sanitizer attributes on DXIL. :) (no action required)

65

(This is already resolved, but in general, strong +1 to more data, less generated code.)

79

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.

beanz added inline comments.Mar 22 2022, 7:18 PM
llvm/lib/Target/DirectX/DXILPrepare.cpp
48

Very much so. I think sanitizer support for HLSL and GPU programs more generally is _very_ interesting :)

beanz updated this revision to Diff 417471.Mar 22 2022, 7:19 PM

Updating to address PR feedback.

beanz updated this revision to Diff 418964.Mar 29 2022, 1:09 PM

Rebasing on main now that the dependent changes have landed.

nikic added inline comments.Mar 31 2022, 3:39 AM
llvm/lib/Target/DirectX/DXILPrepare.cpp
98

Why the three loops here? I think you just need one if you use make_early_inc_range.

llvm/lib/Target/DirectX/DirectX.h
15

This include looks unnecessary given the forward declaration below.

beanz updated this revision to Diff 419501.Mar 31 2022, 9:56 AM
beanz marked 2 inline comments as done.

Updating based on PR feedback from @nikic.

Thank you!

nikic accepted this revision.Apr 1 2022, 8:45 AM

LGTM

This revision is now accepted and ready to land.Apr 1 2022, 8:45 AM
This revision was landed with ongoing or failed builds.Apr 5 2022, 9:50 AM
This revision was automatically updated to reflect the committed changes.