- User Since
- Apr 13 2015, 9:48 AM (249 w, 4 d)
The MBFIWrapper change seems NFC, can it be extracted out first?
ok with me if Reid is ok with windows specific logic.
Wed, Jan 22
Can you extract Window's specific code into its own helper function if possible?
there are checks of explicit occurrences of inline-threshold option later, so the behavior seems unchanged.
ok. I see the intention. As long as the behavior of --inline-threshold option is not changed (it still overrides the new option), the new option seems fine to me. Adding individually controlled option for size opt seems a good idea too.
This change won't work. See
Tue, Jan 21
Mon, Jan 20
Sun, Jan 19
Fri, Jan 17
As other commented, please extract the code into its own function also add the support when AA is available ( as other parts of the function does).
Thu, Jan 16
Wed, Jan 15
thanks for the cleanup. The implicit conversion was not quite readable. LGTM
Tue, Jan 14
InlineResult --> inlining related result (viability, etc) -- it captures two pieces of information: 1) inline decision and 2) when decision is 'no', related inline analysis that leads to the no decision. The class name seems fine. The patch makes the 'decision' part more explicit, and also fixes some bug in missing the right analysis message.
Class Name ResultWithMessage sounds too generic. Why not keeping the InlineResult class name? The rest of the changes look reasonable.
Mon, Jan 13
Fri, Jan 10
Thu, Jan 9
Wed, Jan 8
Tue, Jan 7
The intention is to make the base CallAnalysis becomes a symbolic execution engine (virtual optimizations) what can be reused. The cost tracking is extracted into the derived class.
This looks useful. Is it possible to add a test case?
Mon, Jan 6
this looks good to me. Easwaran, do you have any comments on the refactoring?
Dec 23 2019
This patch will be part of mtrofin's https://reviews.llvm.org/D71733, so there is no need for it.
Dec 20 2019
Use AAManager (include/llvm/Analysis/AliasAnalysis.h) by registering only BasicAA ?
Dec 19 2019
The SROA handling code should also be abstracted away and let the derived class handling cost accumulation. In particular, the common code pattern like:
BasicAA is stateless and should be available and used here to disambiguate.
There is comment like this:
Dec 17 2019
Dec 10 2019
Dec 6 2019
Dec 5 2019
Dec 3 2019
Dec 2 2019
Nov 25 2019
what I mentioned should be complementary to the top-down method in this patch -- it just allows the full top-down to be doable for cross module scenario as well.
One way to handle it is 1) delay early inlining of sites into a hot function if a big percentage of calls to the function are from other modules. This still allows intra module top down inlining of it; or 2) keep a clone of the unlined body of the original function and use that one during cross module inlining.
This looks good. Can this be handled for cross module (thinLTO) case somehow too?
Nov 22 2019
Nov 21 2019
How is the query type going to be checked?
Nov 20 2019
Can this be derived statically from the FuncT or BlockT?
Perhaps just add additional checks on the content of the dumped profile to make the test case more complete.
Nov 19 2019
thanks for the test case.
Nov 15 2019
Since the unlock is the common path, why not replacing goto with a helper as well?
Is it possible to add a test?
Around line 1323 of InlineCost.cpp, there is code that detects if the new indirect callsites can become direct sites after inlining. This information can be passed back to the caller (via GetInlineCost and shouldInline interfaces). WIth that, I think the compile time impact can be minimized.
rebased and addressed review feedbacks.
The compile time impact looks too high. Might consider more fine grained check to trigger more iterations.
Nov 13 2019
Nov 12 2019
Any compile time impact numbers?
/* */ is more common, so perhaps make it consistent to this. lgtm otherwise
Nov 11 2019
Looks good to me. Added Sanjay and Simon for review.
Nov 8 2019
May I suggest another split? DAG.shouldOptForSize change can be split out as a NFC.
Nov 7 2019
If the runtime/size overhead is acceptable, I prefer this simpler version of implementation. We can always document the improvement possibility in the source and enhance it later if needed.
Nov 5 2019
Oct 31 2019
Oct 30 2019
In this case (shown in godbot), why does PM decides to not iterate another round?
Oct 29 2019
Oct 28 2019
With SizeOpts change split, is it ready to split TargetLowering changes out?
Oct 24 2019
Oct 18 2019
Oct 17 2019
If we can clean up the code, that will be great.
maybe we should make lprofCurFilename hidden?
shared lib should have different profile name from the main executable so the test should check that two file names are not identical?
Oct 16 2019
Oct 15 2019
Oct 13 2019
Handling what Wei's case will be a nice thing to have, but it may require more significant change in JT. Currently the JT candidate BB selection is based on checking the conditional value used by branch or return value of ret instr (with this patch).
Oct 12 2019
Wenlei, this sounds like a good idea. Patches are welcome!
Oct 11 2019
JumpThreading is basically basic block cloning followed by control flow simplification. This is just a special case where the second part is missing.