Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DirectX] Revert specialized createOp methods part of #101250 #104245

Conversation

bogner
Copy link
Contributor

@bogner bogner commented Aug 14, 2024

In 8cf8565 "[DirectX] Make DXILOpBuilder's API more useable" we introduced
specialized createOp methods for each DirectX op for convenience, but the API
is buggy and untested. It also isn't actually as useful as I imagined it would
be since we don't have argument names or const-ness represented in DXIL.td
currently. Remove these methods for now and we can reintroduce them if we
actually need them later.

bogner added 2 commits August 15, 2024 00:27
Created using spr 1.3.5-bogner

[skip ci]
Created using spr 1.3.5-bogner
@llvmbot
Copy link
Member

llvmbot commented Aug 14, 2024

@llvm/pr-subscribers-backend-directx

Author: Justin Bogner (bogner)

Changes

In 8cf8565 "[DirectX] Make DXILOpBuilder's API more useable" we introduced
specialized createOp methods for each DirectX op for convenience, but the API
is buggy and untested. It also isn't actually as useful as I imagined it would
be since we don't have argument names or const-ness represented in DXIL.td
currently. Remove these methods for now and we can reintroduce them if we
actually need them later.


Full diff: https://github.com/llvm/llvm-project/pull/104245.diff

2 Files Affected:

  • (modified) llvm/lib/Target/DirectX/DXILOpBuilder.cpp (+1-1)
  • (modified) llvm/lib/Target/DirectX/DXILOpBuilder.h (+1-13)
diff --git a/llvm/lib/Target/DirectX/DXILOpBuilder.cpp b/llvm/lib/Target/DirectX/DXILOpBuilder.cpp
index 42df7c90cb337..91e6931b3f788 100644
--- a/llvm/lib/Target/DirectX/DXILOpBuilder.cpp
+++ b/llvm/lib/Target/DirectX/DXILOpBuilder.cpp
@@ -424,7 +424,7 @@ Expected<CallInst *> DXILOpBuilder::tryCreateOp(dxil::OpCode OpCode,
   return B.CreateCall(DXILFn, OpArgs);
 }
 
-CallInst *DXILOpBuilder::createOp(dxil::OpCode OpCode, ArrayRef<Value *> &Args,
+CallInst *DXILOpBuilder::createOp(dxil::OpCode OpCode, ArrayRef<Value *> Args,
                                   Type *RetTy) {
   Expected<CallInst *> Result = tryCreateOp(OpCode, Args, RetTy);
   if (Error E = Result.takeError())
diff --git a/llvm/lib/Target/DirectX/DXILOpBuilder.h b/llvm/lib/Target/DirectX/DXILOpBuilder.h
index ff66f39a3ceb3..5d83357f7a2e9 100644
--- a/llvm/lib/Target/DirectX/DXILOpBuilder.h
+++ b/llvm/lib/Target/DirectX/DXILOpBuilder.h
@@ -33,25 +33,13 @@ class DXILOpBuilder {
 
   /// Create a call instruction for the given DXIL op. The arguments
   /// must be valid for an overload of the operation.
-  CallInst *createOp(dxil::OpCode Op, ArrayRef<Value *> &Args,
+  CallInst *createOp(dxil::OpCode Op, ArrayRef<Value *> Args,
                      Type *RetTy = nullptr);
 
-#define DXIL_OPCODE(Op, Name)                                                  \
-  CallInst *create##Name##Op(ArrayRef<Value *> &Args, Type *RetTy = nullptr) { \
-    return createOp(dxil::OpCode(Op), Args, RetTy);                            \
-  }
-#include "DXILOperation.inc"
-
   /// Try to create a call instruction for the given DXIL op. Fails if the
   /// overload is invalid.
   Expected<CallInst *> tryCreateOp(dxil::OpCode Op, ArrayRef<Value *> Args,
                                    Type *RetTy = nullptr);
-#define DXIL_OPCODE(Op, Name)                                                  \
-  Expected<CallInst *> tryCreate##Name##Op(ArrayRef<Value *> &Args,            \
-                                           Type *RetTy = nullptr) {            \
-    return tryCreateOp(dxil::OpCode(Op), Args, RetTy);                         \
-  }
-#include "DXILOperation.inc"
 
   /// Return the name of the given opcode.
   static const char *getOpCodeName(dxil::OpCode DXILOp);

bogner added a commit to bogner/llvm-project that referenced this pull request Aug 14, 2024
In 8cf8565 "[DirectX] Make DXILOpBuilder's API more useable" we introduced
specialized createOp methods for each DirectX op for convenience, but the API
is buggy and untested. It also isn't actually as useful as I imagined it would
be since we don't have argument names or const-ness represented in DXIL.td
currently. Remove these methods for now and we can reintroduce them if we
actually need them later.

Pull Request: llvm#104245
Copy link
Member

@hekota hekota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Created using spr 1.3.5-bogner
@bogner bogner changed the base branch from users/bogner/sprmain.directx-revert-specialized-createop-methods-part-of-101250 to main August 16, 2024 19:21
@bogner bogner merged commit 71d54fa into main Aug 16, 2024
1 check was pending
@bogner bogner deleted the users/bogner/sprdirectx-revert-specialized-createop-methods-part-of-101250 branch August 16, 2024 19:21
@bogner
Copy link
Contributor Author

bogner commented Aug 16, 2024

Sorry for the mass review request, it wasn't intentional and I'm not sure what happened there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants