[PATCH] D111703: [ARM] __cxa_end_cleanup should be called instead of _UnwindResume.

[Daniel Kiss via Phabricator via llvm-commits]

danielkiss marked 8 inline comments as done.
danielkiss added inline comments.


================
Comment at: llvm/lib/CodeGen/DwarfEHPrepare.cpp:232
+        TargetTriple.isTargetEHABICompatible()) {
+      RewindName = TLI.getLibcallName(RTLIB::CXA_END_CLEANUP);
+      FTy = FunctionType::get(Type::getVoidTy(Ctx), false);
----------------
logan wrote:
> Add:
> 
> ```
>     RewindFunctionCallingConv = TLI.getLibcallCallingConv(RTLIB::CXA_END_CLEANUP);
> ```
Moved after the `if (!RewindFunction) {` because it need to be done if the RewindFunction is already set. 
RewindFunctionCallingConv  may not worth a class member..


================
Comment at: llvm/lib/CodeGen/DwarfEHPrepare.cpp:248-258
     Value *ExnObj = GetExceptionObject(RI);
-
-    // Call the _Unwind_Resume function.
-    CallInst *CI = CallInst::Create(RewindFunction, ExnObj, "", UnwindBB);
-    CI->setCallingConv(TLI.getLibcallCallingConv(RTLIB::UNWIND_RESUME));
+    CallInst *CI;
+    if (doesRewindFunctionNeedExceptionObject()) {
+      // Call the _Unwind_Resume function.
+      CI = CallInst::Create(RewindFunction, ExnObj, "", UnwindBB);
+      CI->setCallingConv(TLI.getLibcallCallingConv(RTLIB::UNWIND_RESUME));
+    } else {
----------------
logan wrote:
> Refine as:
> 
> ```
>     llvm::SmallVector<Value *, 1> RewindFunctionArgs;
> 
>     if (doesRewindFunctionNeedExceptionObject())
>       RewindFunctionArgs.push_back(GetExceptionObject(RI));
> 
>     // Call the rewind function.
>     CallInst *CI = CallInst::Create(RewindFunction, RewindFunctionArgs, "",
>                                     UnwindBB);
>     CI->setCallingConv(RewindFunctionCallingConv);
> ```
> 
> (Rationale: Avoid repeating `CallInst::Create(...)` and `CI->setCallingConv(...)` for two cases.)
Thanks for the rational, refactored in the spirit of it.
`GetExceptionObject` has sideeffects so I kept the original code structure.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111703/new/

https://reviews.llvm.org/D111703