[HIPIFY][#837][#2062][feature][RFC][Partial] Implement e_insert_new_argument for CUDA-to-HIP APIs with variable declarations - Part 1#2415
Conversation
e_insert_new_argument for CUDA-to-HIP APIs with variable declarations
emankov
left a comment
There was a problem hiding this comment.
Good try on an important feature from our ToDo list, but see my comments, please.
src/HipifyAction.cpp
Outdated
| clang::SourceLocation stmtInsertLoc = callBeginLoc.getLocWithOffset(-static_cast<int>(col - 1)); | ||
| ct::Replacement declRep(SM, stmtInsertLoc, 0, combinedDeclText); |
There was a problem hiding this comment.
The variable declaration logic determines the line start and inserts the new variables directly above the function call.
If the original function call is inside a single-statement control flow block without braces, this textual insertion will break the AST logic.
For instance:
if (status == CUDNN_STATUS_SUCCESS)
cudnnSetDropoutDescriptor(...);will be hipified to smth. like the following:
if (status == CUDNN_STATUS_SUCCESS)
bool use_mask;
bool state_evo;
miopenRNGType_t rng_mode;
miopenSetDropoutDescriptor(...); // This is now outside the if-statement[Recommendation]
To make this robust, check if the parent AST node is a CompoundStmt. If it's not (e.g., it's an IfStmt or ForStmt), you should generate { and } around the inserted declarations and the function call to preserve the original control flow scope.
There was a problem hiding this comment.
Correct, this is a real issue here.
I can implement the AST check in this current PR. It is not only to e_insert_new_argument but it applies to future feature that inserts statements above a call.
it can be designed as a reusable utility with multiple control flow variants like (if, else, for, while, do-while).
Would you prefer I handle it here, or as a dedicated follow-up PR?
80dae85 to
13279bc
Compare
emankov
left a comment
There was a problem hiding this comment.
The middle argument insertion is a very elegant solution!
Using call->getArg(c.first)->getBeginLoc() to seamlessly interleave the new arguments into the existing Clang replacement stream is much cleaner than reconstructing the entire argument list. This is the approach we prefer to use when dealing with arguments in Clang. Adding the {} default initialization is also a great touch to squash uninitialized variable warnings right out of the gate.
However, we need to address the scoping implementation, as it didn't fully resolve the control-flow vulnerability.
13279bc to
750aeb7
Compare
Marking this PR as partial and will create a ToDo for the below
|
e_insert_new_argument for CUDA-to-HIP APIs with variable declarationse_insert_new_argument for CUDA-to-HIP APIs with variable declarations - Part 1
Motivation
Issue #837, additional arguments must be declared before the function call. This is necessary because several cuDNN to MIOpen mappings require parameters such as
use_mask, state_evo, and rng_modeformiopenSetDropoutDescriptorwhich lack CUDA equivalents and cannot be inferred from the current context.Problem Statement
When hipify-clang encounters a CUDA API whose HIP/MIOpen equivalent has additional arguments that are not present in the original call, it currently has no mechanism to
Solution
Introduces
e_insert_new_argument:Summary of changes in this PR:
e_insert_new_argumenttype for APIs where MIOpen requires additional arguments not present in CUDA [HIPIFY][feature] Add a new function call transformation type "additional non-const arg" #837= {}) andhipify_prefixToDo work (follow-up PRs):
if/for/whilewithout braces, the inserted declarations break control flow. Fix: query AST parents to detectCompoundStmtand wrap with{ }.hipify_variables would be declared twice. Fix: append a number (e.g.,hipify_use_mask_1).