[HLSL] Implement output parameter #101083
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang-static-analyzer-1 Author: Chris B (llvm-beanz) ChangesHLSL output parameters are denoted with the For In both cases on return of the function the temporary value is written back to the argument lvalue expression through an optional casting sequence if required. This change introduces a new HLSLOutArgExpr ast node which represents the output argument behavior. The OutArgExpr has two defined children: the base expresion and the writeback expression. The writeback expression will either be or contain an OpaqueValueExpr child expression which is used during code generation to represent the temporary value. Patch is 62.54 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/101083.diff 43 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 6d1c8ca8a2f96..534ffd994cd67 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -1377,6 +1377,15 @@ class ASTContext : public RefCountedBase<ASTContext> {
/// in the return type and parameter types.
bool hasSameFunctionTypeIgnoringPtrSizes(QualType T, QualType U);
+
+ /// Get or construct a function type that is equivalent to the input type
+ /// except that the parameter ABI annotations are stripped.
+ QualType getFunctionTypeWithoutParamABIs(QualType T);
+
+ /// Determine if two function types are the same, ignoring parameter ABI
+ /// annotations.
+ bool hasSameFunctionTypeIgnoringParamABI(QualType T, QualType U);
+
/// Return the uniqued reference to the type for a complex
/// number with the specified element type.
QualType getComplexType(QualType T) const;
diff --git a/clang/include/clang/AST/Attr.h b/clang/include/clang/AST/Attr.h
index 8e9b7ad8b4682..00e3c9d9ab347 100644
--- a/clang/include/clang/AST/Attr.h
+++ b/clang/include/clang/AST/Attr.h
@@ -224,20 +224,7 @@ class ParameterABIAttr : public InheritableParamAttr {
InheritEvenIfAlreadyPresent) {}
public:
- ParameterABI getABI() const {
- switch (getKind()) {
- case attr::SwiftContext:
- return ParameterABI::SwiftContext;
- case attr::SwiftAsyncContext:
- return ParameterABI::SwiftAsyncContext;
- case attr::SwiftErrorResult:
- return ParameterABI::SwiftErrorResult;
- case attr::SwiftIndirectResult:
- return ParameterABI::SwiftIndirectResult;
- default:
- llvm_unreachable("bad parameter ABI attribute kind");
- }
- }
+ ParameterABI getABI() const;
static bool classof(const Attr *A) {
return A->getKind() >= attr::FirstParameterABIAttr &&
@@ -379,6 +366,29 @@ inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
DB.AddTaggedVal(reinterpret_cast<uint64_t>(At), DiagnosticsEngine::ak_attr);
return DB;
}
+
+inline ParameterABI ParameterABIAttr::getABI() const {
+ switch (getKind()) {
+ case attr::SwiftContext:
+ return ParameterABI::SwiftContext;
+ case attr::SwiftAsyncContext:
+ return ParameterABI::SwiftAsyncContext;
+ case attr::SwiftErrorResult:
+ return ParameterABI::SwiftErrorResult;
+ case attr::SwiftIndirectResult:
+ return ParameterABI::SwiftIndirectResult;
+ case attr::HLSLParamModifier: {
+ const auto *A = cast<HLSLParamModifierAttr>(this);
+ if (A->isOut())
+ return ParameterABI::HLSLOut;
+ if (A->isInOut())
+ return ParameterABI::HLSLInOut;
+ return ParameterABI::Ordinary;
+ }
+ default:
+ llvm_unreachable("bad parameter ABI attribute kind");
+ }
+}
} // end namespace clang
#endif
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 5b813bfc2faf9..039719fc1c20d 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -7061,6 +7061,69 @@ class ArraySectionExpr : public Expr {
void setRBracketLoc(SourceLocation L) { RBracketLoc = L; }
};
+/// This class represents temporary values used to represent inout and out
+/// arguments in HLSL. From the callee perspective these parameters are more or
+/// less __restrict__ T&. They are guaranteed to not alias any memory. inout
+/// parameters are initialized by the caller, and out parameters are references
+/// to uninitialized memory.
+///
+/// In the caller, the argument expression creates a temporary in local memory
+/// and the address of the temporary is passed into the callee. There may be
+/// implicit conversion sequences to initialize the temporary, and on expiration
+/// of the temporary an inverse conversion sequence is applied as a write-back
+/// conversion to the source l-value.
+class HLSLOutArgExpr : public Expr {
+ friend class ASTStmtReader;
+
+ Expr *Base;
+ Expr *Writeback;
+ OpaqueValueExpr *OpaqueVal;
+ bool IsInOut;
+
+ HLSLOutArgExpr(QualType Ty, Expr *B, Expr *WB, OpaqueValueExpr *OpV,
+ bool IsInOut)
+ : Expr(HLSLOutArgExprClass, Ty, VK_LValue, OK_Ordinary), Base(B),
+ Writeback(WB), OpaqueVal(OpV), IsInOut(IsInOut) {
+ assert(!Ty->isDependentType() && "HLSLOutArgExpr given a dependent type!");
+ }
+
+ explicit HLSLOutArgExpr(EmptyShell Shell)
+ : Expr(HLSLOutArgExprClass, Shell) {}
+
+public:
+ static HLSLOutArgExpr *Create(const ASTContext &C, QualType Ty, Expr *Base,
+ bool IsInOut, Expr *WB,
+ OpaqueValueExpr *OpV);
+ static HLSLOutArgExpr *CreateEmpty(const ASTContext &Ctx);
+
+ const Expr *getBase() const { return Base; }
+ Expr *getBase() { return Base; }
+
+ const Expr *getWriteback() const { return Writeback; }
+ Expr *getWriteback() { return Writeback; }
+
+ const OpaqueValueExpr *getOpaqueValue() const { return OpaqueVal; }
+ OpaqueValueExpr *getOpaqueValue() { return OpaqueVal; }
+
+ bool isInOut() const { return IsInOut; }
+
+ SourceLocation getBeginLoc() const LLVM_READONLY {
+ return Base->getBeginLoc();
+ }
+
+ SourceLocation getEndLoc() const LLVM_READONLY { return Base->getEndLoc(); }
+
+ static bool classof(const Stmt *T) {
+ return T->getStmtClass() == HLSLOutArgExprClass;
+ }
+
+ // Iterators
+ child_range children() {
+ return child_range((Stmt**)&Base, ((Stmt**)&Writeback) + 1);
+ }
+};
+
+
/// Frontend produces RecoveryExprs on semantic errors that prevent creating
/// other well-formed expressions. E.g. when type-checking of a binary operator
/// fails, we cannot produce a BinaryOperator expression. Instead, we can choose
diff --git a/clang/include/clang/AST/RecursiveASTVisitor.h b/clang/include/clang/AST/RecursiveASTVisitor.h
index e3c0cb46799f7..27c29099c57cf 100644
--- a/clang/include/clang/AST/RecursiveASTVisitor.h
+++ b/clang/include/clang/AST/RecursiveASTVisitor.h
@@ -4014,6 +4014,9 @@ DEF_TRAVERSE_STMT(OpenACCComputeConstruct,
DEF_TRAVERSE_STMT(OpenACCLoopConstruct,
{ TRY_TO(TraverseOpenACCAssociatedStmtConstruct(S)); })
+// Traverse HLSL: Out argument expression
+DEF_TRAVERSE_STMT(HLSLOutArgExpr, {})
+
// FIXME: look at the following tricky-seeming exprs to see if we
// need to recurse on anything. These are ones that have methods
// returning decls or qualtypes or nestednamespecifier -- though I'm
diff --git a/clang/include/clang/AST/TextNodeDumper.h b/clang/include/clang/AST/TextNodeDumper.h
index 39dd1f515c9eb..261853343a011 100644
--- a/clang/include/clang/AST/TextNodeDumper.h
+++ b/clang/include/clang/AST/TextNodeDumper.h
@@ -407,6 +407,7 @@ class TextNodeDumper
void
VisitLifetimeExtendedTemporaryDecl(const LifetimeExtendedTemporaryDecl *D);
void VisitHLSLBufferDecl(const HLSLBufferDecl *D);
+ void VisitHLSLOutArgExpr(const HLSLOutArgExpr *E);
void VisitOpenACCConstructStmt(const OpenACCConstructStmt *S);
void VisitOpenACCLoopConstruct(const OpenACCLoopConstruct *S);
void VisitEmbedExpr(const EmbedExpr *S);
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 46d0a66d59c37..6186161e6b182 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4613,14 +4613,13 @@ def HLSLGroupSharedAddressSpace : TypeAttr {
let Documentation = [HLSLGroupSharedAddressSpaceDocs];
}
-def HLSLParamModifier : TypeAttr {
+def HLSLParamModifier : ParameterABIAttr {
let Spellings = [CustomKeyword<"in">, CustomKeyword<"inout">, CustomKeyword<"out">];
let Accessors = [Accessor<"isIn", [CustomKeyword<"in">]>,
Accessor<"isInOut", [CustomKeyword<"inout">]>,
Accessor<"isOut", [CustomKeyword<"out">]>,
Accessor<"isAnyOut", [CustomKeyword<"out">, CustomKeyword<"inout">]>,
Accessor<"isAnyIn", [CustomKeyword<"in">, CustomKeyword<"inout">]>];
- let Subjects = SubjectList<[ParmVar]>;
let Documentation = [HLSLParamQualifierDocs];
let Args = [DefaultBoolArgument<"MergedSpelling", /*default*/0, /*fake*/1>];
}
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 581434d33c5c9..c499ee8ac5906 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12357,6 +12357,8 @@ def warn_hlsl_availability : Warning<
def warn_hlsl_availability_unavailable :
Warning<err_unavailable.Summary>,
InGroup<HLSLAvailability>, DefaultError;
+def error_hlsl_inout_scalar_extension : Error<"illegal scalar extension cast on argument %0 to %select{|in}1out paramemter">;
+def error_hlsl_inout_lvalue : Error<"cannot bind non-lvalue argument %0 to %select{|in}1out paramemter">;
def err_hlsl_export_not_on_function : Error<
"export declaration can only be used on functions">;
diff --git a/clang/include/clang/Basic/Specifiers.h b/clang/include/clang/Basic/Specifiers.h
index fb11e8212f8b6..0ffd9e06cf3e5 100644
--- a/clang/include/clang/Basic/Specifiers.h
+++ b/clang/include/clang/Basic/Specifiers.h
@@ -382,6 +382,12 @@ namespace clang {
/// Swift asynchronous context-pointer ABI treatment. There can be at
/// most one parameter on a given function that uses this treatment.
SwiftAsyncContext,
+
+ // This parameter is a copy-out HLSL parameter.
+ HLSLOut,
+
+ // This parameter is a copy-in/copy-out HLSL parameter.
+ HLSLInOut,
};
/// Assigned inheritance model for a class in the MS C++ ABI. Must match order
diff --git a/clang/include/clang/Basic/StmtNodes.td b/clang/include/clang/Basic/StmtNodes.td
index 9bf23fae50a9e..a80601b1e4a94 100644
--- a/clang/include/clang/Basic/StmtNodes.td
+++ b/clang/include/clang/Basic/StmtNodes.td
@@ -306,3 +306,6 @@ def OpenACCAssociatedStmtConstruct
: StmtNode<OpenACCConstructStmt, /*abstract=*/1>;
def OpenACCComputeConstruct : StmtNode<OpenACCAssociatedStmtConstruct>;
def OpenACCLoopConstruct : StmtNode<OpenACCAssociatedStmtConstruct>;
+
+// HLSL Constructs.
+def HLSLOutArgExpr : StmtNode<Expr>;
diff --git a/clang/include/clang/Sema/SemaHLSL.h b/clang/include/clang/Sema/SemaHLSL.h
index 2ddbee67c414b..64b565787f325 100644
--- a/clang/include/clang/Sema/SemaHLSL.h
+++ b/clang/include/clang/Sema/SemaHLSL.h
@@ -61,6 +61,8 @@ class SemaHLSL : public SemaBase {
void handleParamModifierAttr(Decl *D, const ParsedAttr &AL);
bool CheckBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall);
+
+ ExprResult ActOnOutParamExpr(ParmVarDecl *Param, Expr *Arg);
};
} // namespace clang
diff --git a/clang/include/clang/Serialization/ASTBitCodes.h b/clang/include/clang/Serialization/ASTBitCodes.h
index 5dd0ba33f8a9c..c19d750d30d56 100644
--- a/clang/include/clang/Serialization/ASTBitCodes.h
+++ b/clang/include/clang/Serialization/ASTBitCodes.h
@@ -1988,6 +1988,9 @@ enum StmtCode {
// OpenACC Constructs
STMT_OPENACC_COMPUTE_CONSTRUCT,
STMT_OPENACC_LOOP_CONSTRUCT,
+
+ // HLSL Constructs
+ EXPR_HLSL_OUT_ARG,
};
/// The kinds of designators that can occur in a
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index a465cdfcf3c89..750928fc00928 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -3590,6 +3590,20 @@ bool ASTContext::hasSameFunctionTypeIgnoringPtrSizes(QualType T, QualType U) {
getFunctionTypeWithoutPtrSizes(U));
}
+QualType ASTContext::getFunctionTypeWithoutParamABIs(QualType T) {
+ if (const auto *Proto = T->getAs<FunctionProtoType>()) {
+ FunctionProtoType::ExtProtoInfo EPI = Proto->getExtProtoInfo();
+ EPI.ExtParameterInfos = nullptr;
+ return getFunctionType(Proto->getReturnType(), Proto->param_types(), EPI);
+ }
+ return T;
+}
+
+bool ASTContext::hasSameFunctionTypeIgnoringParamABI(QualType T, QualType U) {
+ return hasSameType(T, U) || hasSameType(getFunctionTypeWithoutParamABIs(T),
+ getFunctionTypeWithoutParamABIs(U));
+}
+
void ASTContext::adjustExceptionSpec(
FunctionDecl *FD, const FunctionProtoType::ExceptionSpecInfo &ESI,
bool AsWritten) {
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 9d5b8167d0ee6..be12e6e93cc45 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -3631,6 +3631,7 @@ bool Expr::HasSideEffects(const ASTContext &Ctx,
case RequiresExprClass:
case SYCLUniqueStableNameExprClass:
case PackIndexingExprClass:
+ case HLSLOutArgExprClass:
// These never have a side-effect.
return false;
@@ -5318,3 +5319,13 @@ OMPIteratorExpr *OMPIteratorExpr::CreateEmpty(const ASTContext &Context,
alignof(OMPIteratorExpr));
return new (Mem) OMPIteratorExpr(EmptyShell(), NumIterators);
}
+
+HLSLOutArgExpr *HLSLOutArgExpr::Create(const ASTContext &C, QualType Ty,
+ Expr *Base, bool IsInOut, Expr *WB,
+ OpaqueValueExpr *OpV) {
+ return new (C) HLSLOutArgExpr(Ty, Base, WB, OpV, IsInOut);
+}
+
+HLSLOutArgExpr *HLSLOutArgExpr::CreateEmpty(const ASTContext &C) {
+ return new (C) HLSLOutArgExpr(EmptyShell());
+}
diff --git a/clang/lib/AST/ExprClassification.cpp b/clang/lib/AST/ExprClassification.cpp
index 6482cb6d39acc..ebbfaa187263f 100644
--- a/clang/lib/AST/ExprClassification.cpp
+++ b/clang/lib/AST/ExprClassification.cpp
@@ -148,6 +148,7 @@ static Cl::Kinds ClassifyInternal(ASTContext &Ctx, const Expr *E) {
case Expr::ArraySectionExprClass:
case Expr::OMPArrayShapingExprClass:
case Expr::OMPIteratorExprClass:
+ case Expr::HLSLOutArgExprClass:
return Cl::CL_LValue;
// C99 6.5.2.5p5 says that compound literals are lvalues.
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 558e20ed3e423..c692d47ffd1af 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -16469,6 +16469,7 @@ static ICEDiag CheckICE(const Expr* E, const ASTContext &Ctx) {
case Expr::CoyieldExprClass:
case Expr::SYCLUniqueStableNameExprClass:
case Expr::CXXParenListInitExprClass:
+ case Expr::HLSLOutArgExprClass:
return ICEDiag(IK_NotICE, E->getBeginLoc());
case Expr::InitListExprClass: {
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index d46d621d4c7d4..1a1c316b90c4e 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -3507,6 +3507,12 @@ CXXNameMangler::mangleExtParameterInfo(FunctionProtoType::ExtParameterInfo PI) {
case ParameterABI::Ordinary:
break;
+ // HLSL parameter mangling.
+ case ParameterABI::HLSLOut:
+ case ParameterABI::HLSLInOut:
+ mangleVendorQualifier(getParameterABISpelling(PI.getABI()));
+ break;
+
// All of these start with "swift", so they come before "ns_consumed".
case ParameterABI::SwiftContext:
case ParameterABI::SwiftAsyncContext:
@@ -5703,6 +5709,12 @@ void CXXNameMangler::mangleExpression(const Expr *E, unsigned Arity,
Out << "E";
break;
}
+ case Expr::HLSLOutArgExprClass: {
+ const auto *OAE = cast<clang::HLSLOutArgExpr>(E);
+ Out << (OAE->isInOut() ? "_inout_" : "_out_");
+ mangleType(E->getType());
+ break;
+ }
}
if (AsTemplateArg && !IsPrimaryExpr)
diff --git a/clang/lib/AST/StmtPrinter.cpp b/clang/lib/AST/StmtPrinter.cpp
index 69e0b763e8ddc..0a2ac8c16671a 100644
--- a/clang/lib/AST/StmtPrinter.cpp
+++ b/clang/lib/AST/StmtPrinter.cpp
@@ -2799,6 +2799,10 @@ void StmtPrinter::VisitAsTypeExpr(AsTypeExpr *Node) {
OS << ")";
}
+void StmtPrinter::VisitHLSLOutArgExpr(HLSLOutArgExpr *Node) {
+ PrintExpr(Node->getBase());
+}
+
//===----------------------------------------------------------------------===//
// Stmt method implementations
//===----------------------------------------------------------------------===//
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index 89d2a422509d8..37812812ee2b3 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -2631,6 +2631,10 @@ void StmtProfiler::VisitOpenACCLoopConstruct(const OpenACCLoopConstruct *S) {
P.VisitOpenACCClauseList(S->clauses());
}
+void StmtProfiler::VisitHLSLOutArgExpr(const HLSLOutArgExpr *S) {
+ VisitStmt(S);
+}
+
void Stmt::Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
bool Canonical, bool ProfileLambdaExpr) const {
StmtProfilerWithPointers Profiler(ID, Context, Canonical, ProfileLambdaExpr);
diff --git a/clang/lib/AST/TextNodeDumper.cpp b/clang/lib/AST/TextNodeDumper.cpp
index 5ba9523504258..ff88f4aec98a5 100644
--- a/clang/lib/AST/TextNodeDumper.cpp
+++ b/clang/lib/AST/TextNodeDumper.cpp
@@ -2874,6 +2874,10 @@ void TextNodeDumper::VisitHLSLBufferDecl(const HLSLBufferDecl *D) {
dumpName(D);
}
+void TextNodeDumper::VisitHLSLOutArgExpr(const HLSLOutArgExpr *E) {
+ OS << (E->isInOut() ? " inout" : " out");
+}
+
void TextNodeDumper::VisitOpenACCConstructStmt(const OpenACCConstructStmt *S) {
OS << " " << S->getDirectiveKind();
}
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index ffec3ef9d2269..b88e9b8f7f471 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -933,6 +933,10 @@ StringRef clang::getParameterABISpelling(ParameterABI ABI) {
return "swift_error_result";
case ParameterABI::SwiftIndirectResult:
return "swift_indirect_result";
+ case ParameterABI::HLSLOut:
+ return "out";
+ case ParameterABI::HLSLInOut:
+ return "inout";
}
llvm_unreachable("bad parameter ABI kind");
}
@@ -955,7 +959,17 @@ void TypePrinter::printFunctionProtoAfter(const FunctionProtoType *T,
if (EPI.isNoEscape())
OS << "__attribute__((noescape)) ";
auto ABI = EPI.getABI();
- if (ABI != ParameterABI::Ordinary)
+ if (ABI == ParameterABI::HLSLInOut || ABI == ParameterABI::HLSLOut) {
+ OS << getParameterABISpelling(ABI) << " ";
+ if (Policy.UseHLSLTypes) {
+ // This is a bit of a hack because we _do_ use reference types in the
+ // AST for representing inout and out parameters so that code
+ // generation is sane, but when re-printing these for HLSL we need to
+ // skip the reference.
+ print(T->getParamType(i).getNonReferenceType(), OS, StringRef());
+ continue;
+ }
+ } else if (ABI != ParameterABI::Ordinary)
OS << "__attribute__((" << getParameterABISpelling(ABI) << ")) ";
print(T->getParamType(i), OS, StringRef());
@@ -2023,10 +2037,6 @@ void TypePrinter::printAttributedAfter(const AttributedType *T,
case attr::ArmMveStrictPolymorphism:
OS << "__clang_arm_mve_strict_polymorphism";
break;
-
- // Nothing to print for this attribute.
- case attr::HLSLParamModifier:
- break;
}
OS << "))";
}
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 2f3dd5d01fa6c..4835c7ab3e894 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -2830,6 +2830,9 @@ void CodeGenModule::ConstructAttributeList(StringRef Name,
}
switch (FI.getExtParameterInfo(ArgNo).getABI()) {
+ case ParameterABI::HLSLOut:
+ case ParameterABI::HLSLInOut:
+ // FIXME: Do this...
case ParameterABI::Ordinary:
break;
@@ -4148,6 +4151,30 @@ static void emitWriteback(CodeGenFunction &CGF,
assert(!isProvablyNull(srcAddr.getBasePointer()) &&
"shouldn't have writeback for provably null argument");
+ if (CGF.getLangOpts().HLSL) {
+ if (writeback.CastExpr) {
+ RValue TmpVal = CGF.EmitAnyExprToTemp(writeback.CastExpr);
+ if (TmpVal.isScalar())
+ CGF.EmitStoreThroughLValue(TmpVal, srcLV);
+ else
+ CGF.EmitAggregateStore(srcLV.getPointer(CGF),
+ TmpVal.getAggregateAddress(), false);
+ } else {
+ if (srcLV.isSimple())
+ CGF.EmitAggregateStore(srcLV.getPointer(CGF), writeback.Temporary,
+ false);
+ else {
+ llvm::Value *value = CGF.Builder.CreateLoad(writeback.Temporary);
+ RValue TmpVal = RValue::get(value);
+ CGF.EmitStoreThroughLValue(TmpVal, srcLV);
+ }
+ }
+ if (writeback.LifetimeSz)
+ CGF.EmitLifetimeEnd(writeback.LifetimeSz,
+ writeback.Temporary.getBasePointer());
+ return;
+ }
+
llvm::BasicBlock *contBB = nullp...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
HLSL output parameters are denoted with the `inout` and `out` keywords in the function declaration. When an argument to an output parameter is constructed a temporary value is constructed for the argument. For `inout` pamameters the argument is intialized by casting the argument expression to the parameter type. For `out` parameters the argument is not initialized before the call. In both cases on return of the function the temporary value is written back to the argument lvalue expression through an optional casting sequence if required. This change introduces a new HLSLOutArgExpr ast node which represents the output argument behavior. The OutArgExpr has two defined children: the base expresion and the writeback expression. The writeback expression will either be or contain an OpaqueValueExpr child expression which is used during code generation to represent the temporary value.
llvm-beanz
force-pushed
the
cbieneman/inout
branch
from
874a9d5 to
e8ec3e2
Compare
clang/lib/Sema/SemaDecl.cpp
Outdated
| @@ -4057,6 +4057,10 @@ bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD, Scope *S, | |||
| NewQType)) | |||
| return MergeCompatibleFunctionDecls(New, Old, S, MergeTypeWithOld); | |||
|
|
|||
| if (getLangOpts().HLSL && Context.hasSameFunctionTypeIgnoringParamABI( | |||
There was a problem hiding this comment.
Could you explain a bit more what you're doing here? At first glance, functions with different out/inout markings aren't compatible.
There was a problem hiding this comment.
That's correct. We detect and diagnose the mismatch through mergeParamDeclAttributes, which gets called by MergeCompatabileFunctionDecls, so we need to allow the merge call to proceed even for cases here that might be invalid.
Alternatively we could move that diagnostic out somewhere earlier.
There was a problem hiding this comment.
Earlier would be more consistent with how other checks like CheckEquivalentExceptionSpec work, I think?
I'm mostly concerned that this logic is twisted in a way that's hard to understand...
There was a problem hiding this comment.
Can do, will update.
This fixes up the code generation errors in non-scalar writeback operations.
This change moves the verification for parameter ABI qualifier mismatch earlier and only merges declarations if the parameter modifiers are compatible. There is one case where we do need to merge parameter ABI annotations, this is in the case for explicit `in` and implicit `in` (no attribute). All other cases are handled during attribute merging. This addresses feedback from @efriedma-quic, thank you!
clang/lib/Sema/SemaDecl.cpp
Outdated
| if (getLangOpts().HLSL) { | ||
| if (HLSL().CheckCompatibleParameterABI(New, Old)) | ||
| return true; | ||
| return MergeCompatibleFunctionDecls(New, Old, S, MergeTypeWithOld); |
There was a problem hiding this comment.
return MergeCompatibleFunctionDecls(); doesn't look right... I suspect this breaks the check for conflicting types.
Maybe we need to change hasSameFunctionTypeIgnoringExceptionSpec to hasSameFunctionTypeIgnoringExceptionSpecAndParameterABI?
There was a problem hiding this comment.
I wasn't entirely sure about that myself, but couldn't come up with a test case that illustrated a breakage. It is probably a failing of imagination on my part.
hasSameFunctionTypeIgnoringExceptionSpec is used in a bunch of places where I think ignoring the parameter ABI would be a problem. Probably not for HLSL because HLSL doesn't support exceptions, but maybe for other cases that use parameter ABIs (swift interop).
I'll put up a patch shortly that brings back the ASTContext::hasSameFunctionTypeIgnoringParamABI, and uses that in conjunction with hasSameFunctionTypeIgnoringExceptionSpec here to be conservative. Does that sound reasonable?
This change makes it so that function declarations are only merged when no parameter ABI mismatches occur and when the functions have the same type ignoring the paramater ABIs. This allows handling cases where implict and explicit parameter ABIs are the same as with the implicit in and explict in for HLSL.
One of these was left behind to properly put in `noalias` call argument attributes, the other was there while I was working out the writeback logic and is resolved and verified by the tests.
We should be annotating the parameter types as restrict references. This allows the alias information within a function to correctly identify non-aliasing parameters.
Co-authored-by: Damyan Pepper <[email protected]>
I swear I know my A, B, C's...
|
@rjmccall curious if you have some time to look at this PR. I've made some small changes to extend parameter ABI annotations and write back arguments (used by ObjC) to cover HLSL's odd parameter behaviors. |
clang/include/clang/AST/Expr.h
Outdated
|
|
||
| // Iterators | ||
| child_range children() { | ||
| return child_range((Stmt **)&Base, ((Stmt **)&Writeback) + 1); |
There was a problem hiding this comment.
I certainly hope we don't do this elsewhere. Please declare an array field and make the accessors pull the elements out.
clang/lib/AST/ItaniumMangle.cpp
Outdated
| Out << (OAE->isInOut() ? "_inout_" : "_out_"); | ||
| mangleType(E->getType()); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Okay, so this is wrong in every way. :)
Expression mangling generally follows the syntax. Since there is no argument-side syntax for passing an argument to an out or inout parameter, I'd say the correct mangling is just to ignore this node and mangle the underlying (syntactic) argument expression, which I assume is fairly easy to find (is that the base expression? whatever doesn't have the OVE still in it).
If it were important to mangle this for some reason, you would want to mangle it as a vendor extended expression, u <source-name> <template-arg>* E.
| if (auto *OutArgE = dyn_cast<HLSLOutArgExpr>(E)) { | ||
| // The base expression is only used to initialize the parameter for | ||
| // arguments to `inout` parameters, so we only traverse down the base | ||
| // expression for `inout` cases. |
There was a problem hiding this comment.
You will need to recurse into the original l-value even for an out.
There was a problem hiding this comment.
I think you're still missing this.
There was a problem hiding this comment.
I should have commented on this. If SemaChecking traverses down both sides we get warnings for the conversion that would "initialize" the out parameter, but those warnings aren't accurate because no initialization occurs.
If we only traverse down the write back side we only get the warning for the copy back to the l-value, which is the behavior we want.
clang/test/SemaHLSL/Language/OutputParameters.hlsl verifies the behavior for out arguments of only diagnosing the write back conversion, removing the condition breaks that test by emitting diagnostics on the initialization as well.
There was a problem hiding this comment.
I specifically said the original l-value expression. I'm trying to make sure we get warnings in sub-expressions of the l-value that have nothing to do with the final conversion to produce the argument. Consider, e.g., implicit conversions in the index expression of array[E].
With that said:
If SemaChecking traverses down both sides we get warnings for the conversion that would "initialize" the out parameter, but those warnings aren't accurate because no initialization occurs.
Isn't this an argument that you either (1) shouldn't visit the initialization specifically for out, but should still do it for inout, or (2) shouldn't generate the initialization expression for out?
Apropos, I know this is a future direction, but what semantics do you want for C++ class types and out? Skipping the initialization entirely is definitely incorrect — the callee will get passed a reference to an uninitialized object and (presumably) start calling methods on it. Probably this should default-initialize?
There was a problem hiding this comment.
Gotcha! I think I misunderstood your comment since I didn't have the original lvalue preserved initially. I'll update this.
These are pretty wonky when we start looking at C++isms. My team reacted with disgust when I posed this potential (but currently invalid) code:
RWBuffer<int> Val;
struct A {
int Idx;
A &operator ++() {
Val[Idx] = Val[Idx] + 1;
return *this;
}
};
void init(out A Obj) {
Obj.Idx = 2;
};
[numthreads(1,1,1)]
void main() {
A a;
a.Idx = 0;
init(++a);
++a;
}We certainly need to evaluate the argument expression (so that it can side-effect). Constructors would make this more complicated, but it probably should default initialize. Are you okay with that being a follow-up? If so I'll file an issue to track doing that work.
There was a problem hiding this comment.
Yeah, since you're not supporting C++ types here, it's fine to leave the work for later.
clang/include/clang/AST/Expr.h
Outdated
| bool IsInOut, Expr *WB, OpaqueValueExpr *OpV); | ||
| static HLSLOutArgExpr *CreateEmpty(const ASTContext &Ctx); | ||
|
|
||
| const Expr *getBase() const { return Base; } |
There was a problem hiding this comment.
You need to document what the expressions are here, because it's pretty subtle. So the way you've written it, this a conversion of the base l-value to the type of the parameter, the writeback is an assignment of the temporary result back to the base l-value, and the OVE represents the temporary result. I think this is problematic, though; see the comment in Sema.
clang/lib/Sema/SemaExpr.cpp
Outdated
| @@ -5915,6 +5916,16 @@ bool Sema::GatherArgumentsForCall(SourceLocation CallLoc, FunctionDecl *FDecl, | |||
| ProtoArgType->isBlockPointerType()) | |||
| if (auto *BE = dyn_cast<BlockExpr>(Arg->IgnoreParenNoopCasts(Context))) | |||
| BE->getBlockDecl()->setDoesNotEscape(); | |||
| // During template instantiation it is possible that we already created | |||
| // the HLSLOutArgExpr if it was produced during tree transformation. | |||
There was a problem hiding this comment.
Generally, TreeTransform just drops these implicit nodes on the floor, instantiates the original expression, and creates new nodes when it's re-processing the instantiated call. This can get complicated around OVEs for e.g. pseudo-object expressions, but in your case, it should be easy because all of the extra structure is implicit stuff, and we haven't restructured anything.
There was a problem hiding this comment.
Since you've fixed TreeTransform to look through the HLSLOutArgExpr, it should now no longer be possible to see an HLSLOutArgExpr here.
clang/lib/Sema/SemaHLSL.cpp
Outdated
| } | ||
|
|
||
| bool RequiresConversion = | ||
| Ty.getUnqualifiedType() != Arg->getType().getUnqualifiedType(); |
There was a problem hiding this comment.
This is actually doing a non-canonical comparison, which is not what you want; the test case is just writing one of these with a typedef of the type of the other. There's a ASTContext::hasSameUnqualifiedType that you should use.
clang/lib/Sema/SemaHLSL.cpp
Outdated
| // results in a copy we can ignore qualifiers. | ||
| if (RequiresConversion) { | ||
| ExprResult Res = | ||
| SemaRef.PerformImplicitConversion(Arg, Ty, Sema::AA_Passing); |
There was a problem hiding this comment.
Okay. It looks like you're losing track of Arg here. I honestly don't understand how CodeGen works in the converted case; it seems like it'd have to be wrong, because you're calling EmitLValue on Base when it isn't even an l-value anymore.
The most obvious representation would be something like this:
- Make an OVE for the original l-value (
Arg). You can useArgas the source expression. This is always an l-value. - Make a conversion from that OVE to the parameter type. This is always an r-value.
- Make an OVE for the temporary value; this is always an l-value and should not have a source expression.
- Make a conversion from that OVE to the original l-value type. This is always an r-value.
You could potentially try to recover the original l-value by looking through the implicit conversions on the converted r-value expression. In C use cases, that might be reliable; you could assert here that your algorithm works. That would be simpler for the representation, but probably significantly harder in CodeGen without double-emitting the original l-value.
HLSL is a C extension and doesn't support C++, right? So you don't need to worry about user-defined assignment operators for the writeback?
There was a problem hiding this comment.
HLSL does support some C++ features. It doesn't (yet) support user-defined assignment operators, casting operators, or constructors, but those are all highly requested features.
There was a problem hiding this comment.
Okay. If you want to future-proof for that, then for inout, you probably want to generate the writeback expression using ActOnBinOp on your two OVEs (for inout). You can implicitly cast the RHS to an x-value, the way a static_cast would. That probably simplifies the work you have to do in IRGen as well because you don't have to manually perform the assignment.
For out, you'd need to emit a construction expression, as if this were an initializer; so the cases start to diverge pretty heavily there.
There was a problem hiding this comment.
My latest update is mostly in line with the feedback here. I did the OVE's a little different, but it radically simplified the CodeGen changes (thank you for all the suggestions!).
I've changed the initialization to use PerformCopyInitialization so that it will resolve constructors correctly and the write back is just a binary op built with ActOnBinOp. That also radically simplified the codegen.
At a simplified level the C++-equivalent code for how inout and out arguments work is something like this:
namespace detail {
template<typename TmpT, typename SrcT>
struct InOutAdapter {
SrcT &SrcLV;
TmpT TmpLV;
InOutAdapter(SrcT &S) : SrcLV(S), TmpLV(S) {}
~InOutAdapter() { SrcLV = TmpLV; }
operator TmpT&(){
return TmpLV;
}
};
}
template <typename ArgT, typename SrcT>
detail::InOutAdapter<ArgT, SrcT> make_inout(SrcT &V) {
return detail::InOutAdapter<ArgT, SrcT>(V);
}
namespace detail {
template<typename TmpT, typename SrcT>
struct OutAdapter {
SrcT &SrcLV;
TmpT TmpLV;
OutAdapter(SrcT &S) : SrcLV(S) {}
~OutAdapter() { SrcLV = TmpLV; }
operator TmpT&(){
return TmpLV;
}
};
}
template <typename ArgT, typename SrcT>
detail::OutAdapter<ArgT, SrcT> make_out(SrcT &V) {
return detail::OutAdapter<ArgT, SrcT>(V);
}I believe with my latest changes the AST generation and codegen both correctly match this.
llvm-beanz
requested review from
dcci,
ftynse,
makslevental and
stellaraccident
as code owners
llvm-beanz
force-pushed
the
cbieneman/inout
branch
from
fc81a6b to
5f7e351
Compare
# Conflicts: # clang/include/clang/Sema/SemaHLSL.h # clang/lib/Sema/SemaHLSL.cpp
| /// and the address of the temporary is passed into the callee. There may be | ||
| /// implicit conversion sequences to initialize the temporary, and on expiration | ||
| /// of the temporary an inverse conversion sequence is applied as a write-back | ||
| /// conversion to the source l-value. |
There was a problem hiding this comment.
Somewhere in this declaration — either here or on the getter/setter pairs below — you need to actually document the structure of the sub-expressions. That's needed whenever the sub-expressions don't obviously reflect the structure of the source code, which at minimum is true whenever an OVE is involved.
clang/lib/CodeGen/CGExpr.cpp
Outdated
|
|
||
| LValue TempLV; | ||
|
|
||
| if (E->isInOut()) { |
There was a problem hiding this comment.
We can simplify this further, I think — let's just make a temporary and then emit the initialization into it. There's a function in CGExprAgg called EmitInitializationToLValue that you can steal for the second half of that; you'll just need to cut the dependency on Dest.isZeroed() by passing it in as a flag.
There was a problem hiding this comment.
I was hoping you'd extract that as a helper function you'd just call rather than duplicating the logic here.
| if (auto *OutArgE = dyn_cast<HLSLOutArgExpr>(E)) { | ||
| // The base expression is only used to initialize the parameter for | ||
| // arguments to `inout` parameters, so we only traverse down the base | ||
| // expression for `inout` cases. |
There was a problem hiding this comment.
I think you're still missing this.
clang/lib/CodeGen/CGExpr.cpp
Outdated
| @@ -5432,6 +5434,57 @@ LValue CodeGenFunction::EmitOpaqueValueLValue(const OpaqueValueExpr *e) { | |||
| return getOrCreateOpaqueLValueMapping(e); | |||
| } | |||
|
|
|||
| LValue CodeGenFunction::BindHLSLOutArgExpr(const HLSLOutArgExpr *E, | |||
There was a problem hiding this comment.
This seems to be dead now.
| @@ -8511,7 +8514,7 @@ static void HandleHLSLParamModifierAttr(QualType &CurType, | |||
| return; | |||
| if (Attr.getSemanticSpelling() == HLSLParamModifierAttr::Keyword_inout || | |||
| Attr.getSemanticSpelling() == HLSLParamModifierAttr::Keyword_out) | |||
| CurType = S.getASTContext().getLValueReferenceType(CurType); | |||
| CurType = S.HLSL().getInoutParameterType(CurType); | |||
There was a problem hiding this comment.
There are semantic restrictions here, right? If nothing else, CurType can't be a reference type.
There was a problem hiding this comment.
HLSL doesn't allow references or pointers (at least not explicitly). We generate references in the AST in a few places, but any attempt to write or type-deduce one results in an error in Sema.
In theory, any legal type in HLSL can be converted to an inout parameter type.
There was a problem hiding this comment.
You can't reseat a reference, so I don't know what it would mean to pass one inout. It should probably be invalid.
At any rate, if HLSL doesn't allow references, that's fine; please document that expectation with a comment, though (and maybe an assertion that we never see a reference here?).
There was a problem hiding this comment.
I put an assert in SemaHLSL::getInoutParameterType so that it will fire anywhere we try to adjust a type to the inout/out argument type.
Also got a few things I missed in my last round of updates.
This processes just the initial lvalue during sema checking since that expression is always evaluated. The updated test case has an implicit conversion in the lvalue expression, which is evaluated but not used to initialize the argument.
This updates the code in SemaHLSL::getInoutParameterType to assert on reference types, and fixes the cooresponding code in SemaType so that the underlying type isn't mutated more than once. The double-mutation only occured in error cases where conflicting attributes were applied (error is checked elsewhere).
|
ping @rjmccall. I think I've updated to address all your feedback. Let me know if there's anything else you think I should change. |
clang/include/clang/AST/Expr.h
Outdated
| /// sequence from the source lvalue to the argument type. | ||
| /// - A BinaryOperatorExpr that assigns the first sub-expression with the | ||
| /// value from an implict conversion sequence from the second expression to | ||
| /// the argument expression's type. |
There was a problem hiding this comment.
Maybe just "An expression that assigns the second expression into the first, performing any necessary conversions."
clang/lib/CodeGen/CGCall.h
Outdated
|
|
||
| /// An Expression representing a cast from the temporary's type to the | ||
| /// source l-value's type. | ||
| const Expr *CastExpr; |
There was a problem hiding this comment.
This comment is no longer correct; consider something like WritebackExpr. And maybe the logic for writebacks should simply be that we evaluate this expression if it's present, rather than making a language-mode decision?
clang/lib/CodeGen/CGExpr.cpp
Outdated
|
|
||
| LValue TempLV; | ||
|
|
||
| if (E->isInOut()) { |
There was a problem hiding this comment.
I was hoping you'd extract that as a helper function you'd just call rather than duplicating the logic here.
| if (auto *OutArgE = dyn_cast<HLSLOutArgExpr>(E)) { | ||
| // The base expression is only used to initialize the parameter for | ||
| // arguments to `inout` parameters, so we only traverse down the base | ||
| // expression for `inout` cases. |
There was a problem hiding this comment.
Yeah, since you're not supporting C++ types here, it's fine to leave the work for later.
clang/lib/Sema/SemaExpr.cpp
Outdated
| @@ -5915,6 +5916,16 @@ bool Sema::GatherArgumentsForCall(SourceLocation CallLoc, FunctionDecl *FDecl, | |||
| ProtoArgType->isBlockPointerType()) | |||
| if (auto *BE = dyn_cast<BlockExpr>(Arg->IgnoreParenNoopCasts(Context))) | |||
| BE->getBlockDecl()->setDoesNotEscape(); | |||
| // During template instantiation it is possible that we already created | |||
| // the HLSLOutArgExpr if it was produced during tree transformation. | |||
There was a problem hiding this comment.
Since you've fixed TreeTransform to look through the HLSLOutArgExpr, it should now no longer be possible to see an HLSLOutArgExpr here.
Co-authored-by: John McCall <[email protected]>
Co-authored-by: John McCall <[email protected]>
(apparently I forgot to save this file)
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/143/builds/1833 Here is the relevant piece of the build log for the reference |
HLSL output parameters are denoted with the
inoutandoutkeywords in the function declaration. When an argument to an output parameter is constructed a temporary value is constructed for the argument.For
inoutpamameters the argument is intialized by casting the argument expression to the parameter type. Foroutparameters the argument is not initialized before the call.In both cases on return of the function the temporary value is written back to the argument lvalue expression through an optional casting sequence if required.
This change introduces a new HLSLOutArgExpr ast node which represents the output argument behavior. The OutArgExpr has two defined children: the base expresion and the writeback expression. The writeback expression will either be or contain an OpaqueValueExpr child expression which is used during code generation to represent the temporary value.
Fixes #87526