This is an archive of the discontinued LLVM Phabricator instance.

[DirectX backend] Add pass to lower llvm intrinsic into dxil op function.
ClosedPublic

Authored by python3kgae on May 2 2022, 2:10 PM.

Details

Summary

A new pass DXILOpLowering was added.
It will scan all llvm intrinsics, create dxil op function if it can map to dxil op function.
Then translate call instructions on the intrinsic into call on dxil op function.
dxil op function will add i32 argument to the begining of args for dxil opcode.
So cannot use setCalledFunction to update the call instruction on intrinsic.

This commit only support sin to start the work.

Diff Detail

Event Timeline

python3kgae created this revision.May 2 2022, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 2:10 PM
python3kgae requested review of this revision.May 2 2022, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 2:10 PM

Overall approach looks good to me, I just have a whole bunch of comments on the details.

llvm/lib/Target/DirectX/DXILOpLowering.cpp
33–34

Non-trivial globals require an initializer that is executed when the libLLVM.so (or DLL) is loaded, which is a negative impact on a huge number of users of LLVM, including those not using the DirectX target.

I'd suggest making this a static function-local variable.

163–166

Please do TableGen this. When you do, please consider replacing the inline const char* by an offset into a large string constant. This has two benefits:

  1. It allows you to pack the struct more tightly, using a 32-bit or even 16-bit index instead of a 64-bit pointer.
  2. It avoids the need for constant data relocations when loading libLLVM.so.

I'm okay with keeping the FIXME comment for now.

174–179

I believe you can use llvm::lower_bound(OpCodeProps, ...) here.

180–182

Just *Pos? Or rename Pos to Prop?

217–219

Nit: You don't need braces around single-line if bodies, and personally I think this is more readable without.

234–236

You may want to already make this code ready for use with the NewPassManager.

llvm/test/CodeGen/DirectX/sin.ll
2

Could this be an opt test?

python3kgae marked 7 inline comments as done.

Update based on comments.

kuhar added inline comments.May 8 2022, 7:40 PM
llvm/lib/Target/DirectX/DXILOpLowering.cpp
32

nit: prefer StringLiteral

34

nit: prefer enum class

84

Use cast if you know that the cast must succeed
also nit: don't prefix pointer variables with p

95

Can this be some other value than these cases, e.g., 128 or 63 bits? Should we have a default case with llvm_unreachable?

122–123

nit: you can do os.str() which will flush for you.

144

Shouldn't we check that this array element can be accessed?

150

I think you can do Twine(x) + y, without creating a second twine. If not, consider changing char * to StringLiteral

158–161

Could we *not* introduce 2/3-letter-long acronyms like this? I think this hurts readability unless such types are very well known.

174

nit: use llvm::lower_bound

176

I'm confused by this division. Can you pull this out into a local variable with a descriptive name and/or add a comment?

188

nit: can you compare to zero instead of implicitly converting to bool and negating?

193

nit: llvm prefers !ptr instead of ptr == nullptr. Also, please capitalize the assertion message.

198

Why exactly 4? If we don't know the expected size, prefer the automatic one with SmallVector<Type *>

211–212

nit: Could we make_early_inc_range instead?

214

nit: !CI

216

Why do we need a second builder?

217

same here

230

I think SmallDenseMap would work here

232–233

also here

kuhar added inline comments.May 8 2022, 7:43 PM
llvm/lib/Target/DirectX/DXILOpLowering.cpp
151
python3kgae marked 19 inline comments as done.

Update to fix commented issue.

llvm/lib/Target/DirectX/DXILOpLowering.cpp
34

enum class will require cast on case like OverloadKind::FLOAT | OverloadKind::HALF.
I prefer not to have
(uint16_t)OverloadKind::FLOAT | (uint16_t)OverloadKind::HALF.

Is it OK to keep it without enum class?

kuhar added inline comments.May 10 2022, 10:01 AM
llvm/lib/Target/DirectX/DXILOpLowering.cpp
34

There are 3 options:

  1. Keep the code as-is
  2. Add enum class and cast manually
  3. Add enum class and overload the operators you need and do the casting inside.

I think both 1 and 3 are fine. 1 is less code and this enum is not in a header, so I'd be leaning towards keeping this as-is.

Rebase to avoid merge conflict.

kuhar accepted this revision.May 10 2022, 10:06 AM

LGTM but please get another one before submitting

llvm/lib/Target/DirectX/DXILOpLowering.cpp
122
This revision is now accepted and ready to land.May 10 2022, 10:06 AM

Use static instead of namespace.

beanz accepted this revision.May 10 2022, 6:08 PM

Two small nitpicks, otherwise LGTM.

llvm/lib/Target/DirectX/DXILOpLowering.cpp
64

Rather than a default case here, fill in all possible enum values (I think you are just missing void, user-defined, and object). Covering all cases will result in a diagnostic if new cases are added and not handled.

193

nit: exists

Update for comments.

This revision was landed with ongoing or failed builds.May 11 2022, 12:03 AM
This revision was automatically updated to reflect the committed changes.