- User Since
- Jan 15 2018, 8:31 AM (39 w, 2 d)
Fri, Oct 12
LGTM, thanks for fixing this
Beyond constructors/destructors I believe an API which we implement through access to dynamic symbols for global variable is clGetProgramBuildInfo(..., CL_PROGRAM_BUILD_GLOBAL_VARIABLE_TOTAL_SIZE, ...) which is defined as "The total amount of storage, in bytes, used by program variables in the global address space."
That's my understanding, at least.
The rationale is that -fvisibility only affects the default, and already does not apply in many cases. For example, see the rest of the conditions above the fvisibility check in getLVForNamespaceScopeDecl: when Var->getStorageClass() == SC_Static the linkage is (usually) internal and the visibility is default. In cases where individual symbols need unique visibility an __attribute__ can be used.
Thu, Oct 11
Wed, Oct 10
Rebase and remove redundant qualifiers
Cleanup patch is at https://reviews.llvm.org/D53088
Tue, Oct 9
I can submit a separate patch to clean up things like making the empty definitions into declarations, removing redundant public: qualifiers, and removing redundant references to this, but I don't want to mix those changes into this patch.
Mon, Oct 8
Remove update of dyn_cast in WebAssemblyFixFunctionBitcasts which does not use CallSite
I will update the patch to modify the HIP toolchain and to add tests for global variables.
Thu, Oct 4
I don't know who else to add as a reviewer; Sam, is there someone else outside of AMD that would be interested in reviewing this?
Wed, Oct 3
Remove -debug flag from test
Add test using IndirectCallPromotion
Tue, Oct 2
Thanks, Sam. I think the requirements of the two passes are different enough that combining them is not particularly useful.
Move CallPromotionUtils changes to new review (https://reviews.llvm.org/D52792)
Sorry, moved a little fast and grabbed the wrong patch at first.
After reading the WebAsm code more closely it seems like our goals are different. They seem to support indirect calls, but only if certain properties of the functions match (namely number of arguments and return type). For example, the existing pass does not transform anything (e.g. leaves a bitcast function call present) if the bitcast only modifies argument types in a trivial way. For AMDGPU we need to always eliminate the bitcast, and we don't necessarily need to be generating wrapper functions.
The WebAsm code essentially does the same thing, but also creates wrapper functions when the number of arguments differ, or when the return value differs (it seems like this could just be a cast as well). I will move the WebAsm pass to lib/Transform and use it in both backends. I am not sure who from WebAsm to ask to take a look once I'm done?
Mon, Oct 1
@mssimpso Are you OK with reviewing the changes to CallPromotionUtils.cpp in this patch? I wasn't sure who the correct code owner was.
This patch requires changes to CallGraph, and doesn't address inlining through bitcasts. I think https://reviews.llvm.org/D52741 can supercede it, and if we still want to make those changes to CallGraph they can be done independently.
Mon, Sep 24
Sep 14 2018
Address feedback: make MDT optional, add braces, run clang-format.
Sep 12 2018
It looks like to make this change we need to update CallGraph as well; it currently just uses .getCalledFunction(), so the graph doesn't have the edge created by the bitcasted call.
Sep 11 2018
LGTM, but adding Matt because I think he was the one to originally request eliding defaulted fields. From what Tony described I agree we can't do this generally because of options which depend on attributes, e.g. -mattr=+/-xnack changes the "default".
Seems to be related to the order; this succeeds:
I still see asserts related to bitcasted function calls, but I haven't gotten to the bottom of why; an example:
Sep 10 2018
Update patch to include all changes since the original patch was reverted.
Sorry; I just checked again and llc had not been rebulit for some reason. I deleted it and ran the build again and that example does compile fine.
Sep 7 2018
I still don't understand exactly when a call is indirect, but when an argument is cast this still seems to fail:
Sep 6 2018
Update existing tests RUN lines with -verify-machine-dom-info. This replicates what the expensive tests enabled.
The regression was caught by the expensive_checks build, if that is sufficient for testing? I am actually not sure how to write a lit test for this, because opt -analyze will just re-calculate the DT; I don't know how to just print the DT after the legalize pass runs.
Review for fix of that regression is at https://reviews.llvm.org/D51742
Sep 4 2018
Aug 31 2018
Update test name to be clear it only runs when aarch64 is available.
Added test for shared ownership. I've run it locally with ASan and UBSan enabled.
Aug 30 2018
The use I have in mind is actually an external component which depends on LLVM. We currently implement reading MessagePack metadata there, but when we land the changes to generate it in LLVM it would be nice to avoid duplicating the code there.
Aug 29 2018
+Matt to confirm, but our executable format is a shared object and we eventually want to support preemptible symbols through the PLT. We already generate GOT entries for globals.
Aug 28 2018
I've updated the patch to just support shared_ptr; I think the more generic option is right for now.
Address feedback. There was no particular reason for not writing these inline, other than to prevent typos. With the verifier and tests we have I don't think that is an issue, and I agree these being grepable is worthwhile.
Add virtual destructor to base class and clean up tests.
Default the test case destructor rather than give it an empty body.
Aug 27 2018
In cleaning up this patch I noticed our YAML parser does not handle an explicit document containing only a block-level plain scalar, e.g.:
Aug 24 2018
Clear killed flag on uses, add test at -O0
Rebase and ping
Modular build and UB fixes (r340507) are committed.
Aug 23 2018
I think even in the simplest case my code is wrong. The $soffset operand to the BUFFER_LOAD remains killed but the def is not moved into the LoopBB. I am essentially transforming:
Aug 22 2018
Thank you both! I will try to keep modules in mind in the future.