In D118440#3428670, @RKSimon wrote:

@ebrevnov I've reverted this as it was breaking a large number of buildbots due to link errors, maybe you've added a new lib dependency? https://lab.llvm.org/buildbot/#/builders/139/builds/19686

Yep, there is a new dependency which I didn't catch because I use dynamic linking locally. Fixed that quickly but your revert reached first :-)

I assume there are no more comments so far. If you find anything, please let me know and I will fix.

Typo fixed Residal->Residual

Rebase + formatting

Looks good now?

Rebase

Update

Updated

The latest one (4a0481e981b653153fd07db9cbee1197ecd388d3 ) helps. Thanks!

Thanks, I will check tomorrow.

Could you point out to a fixing commit?

Hi Florian,

The ball is on my side but I can't return to it due to other higher priority tasks. Please let me know if there is a demand from your side so I could re-prioritize this one. Also, as mentioned earlier, the help is more than welcome :-)

I'm extending functionality of memory lowering utilities which meant to be used in different context. At the moment there is no any upstream pass which will take advantage of added functionality immediately. Rather, we have downstream functionality which will make use of it. In future, upstream users can decide to utilize the functionality on case by case bases... I don't have enough expertise in those areas to drive changes

In D118441#3296826, @arsenm wrote:

In D118441#3292872, @ebrevnov wrote:

In D118441#3289224, @arsenm wrote:

In D118441#3286321, @ebrevnov wrote:

In D118441#3283987, @arsenm wrote:

Needs some lit tests that show the metadata after expansion

The problem is there is now pass which runs this utility. I'm not aware how to exercise it from lit test. For that reason I had to create a unit test. Is there a way to build a lit test in such case?

AMDGPU uses this, see llvm/test/CodeGen/AMDGPU/lower-mem-intrinsics.ll

In order for this to work I would need to modify AMDGPULowerIntrinsics pass to provide additional argument(result of ScalarEvolutionAnalysis) to expandMemCpyAsLoop. Currently, this pass doesn't depend on SCEV analysis.

That's fine

Rebase

Fixed according to comments

Updated

In D118441#3289224, @arsenm wrote:

In D118441#3286321, @ebrevnov wrote:

In D118441#3283987, @arsenm wrote:

Needs some lit tests that show the metadata after expansion

The problem is there is now pass which runs this utility. I'm not aware how to exercise it from lit test. For that reason I had to create a unit test. Is there a way to build a lit test in such case?

AMDGPU uses this, see llvm/test/CodeGen/AMDGPU/lower-mem-intrinsics.ll

In order for this to work I would need to modify AMDGPULowerIntrinsics pass to provide additional argument(result of ScalarEvolutionAnalysis) to expandMemCpyAsLoop. Currently, this pass doesn't depend on SCEV analysis.

In D118441#3283987, @arsenm wrote:

Needs some lit tests that show the metadata after expansion

Source and destionation of atomic memcopy may not overlap by sepcification

This change implies that speculation is not beneficial regardless whether block is predicted to be taken or untaken. That essentially means that the optimization is not beneficial in most cases of unpredicted (at compile time) branch as well since hardware will actually predict well in most cases. While there is an explicit note that it's hard to do proper cost modeling on IR level there are still some simple cases which are beneficial. So I think we will regress in some cases.

Ping

Ping

Ping

In D117095#3243989, @reames wrote:

@ebrevnov Thanks for the explanation, that makes a lot more sense!

Your explanation raises a question for me. Have we ever defined the semantics of operand bundles on intrinsic calls? We've certainly done so for arbitrary calls, but intrinsics are somewhat "special". The whole point of them is that they have known semantics. This is purely a "I don't remember" question, the answer may be obvious. I'm just hoping for reminder/pointer to something to spark my memory.

Don't know either. As I can judge from common logic and implementation of "CallBase::hasReadingOperandBundles()" (especially comment) the same rules should be applied to intrinsics.

Fix comment

Thank you all for your input. The reason why generic argmemonly case (pointed by @nikic) doesn't handle my case is presence of deopt bundle on the call. Currently, "BasicAAResult::getModRefBehavior(const CallBase *Call)" simply ignores function attributes in presence of deopt bundles. By specification: "From the compiler’s perspective, deoptimization operand bundles make the call sites they’re attached to at least readonly. They read through all of their pointer typed operands (even if they’re not otherwise escaped) and the entire visible heap". That has two consequences:

1. Current code which special case AnyMemCpyInst (the one I was trying to extend to any memory transfer) is incorrect because it may return NoModRef if memcpy intrinsic has operand bundles. Should it be fixed or just removed (can remove only once the next item is resolved though)?
2. It's impossible to describe memcpy/memmove modref behavior with existing encoding defined by FunctionModRefBehavior. In particular, for memmove/memcopy with deopt bundle we need to encode "reads everything, writes inaccessible or argmemonly". Current scheme doesn't support setting different modref for individual type of memory. The best solution which came to me so far is to split available space (currently it's 32-bits given by "enum FunctionModRefBehavior") to 4 regions (4 regions is enough to represent 8 variants defined by "enum class ModRefInfo") and associate each region with specific modref value from ModRefInfo enum. With this approach we can represent any variation for 8 independent memory types in 32-bits which seems to be enough (assuming modref info won't change). In case that turns out to be insufficient can easily double number of supported attributes by switching to 64-bits.

To make it more clear I can give an example. Say we need to encode "reads everything, writes inaccessible or argmemonly". Assume we split our 32-bits by regions in the following way "MustMod | MustRef | Mod | Ref" (each region is 4-bits). Then the value would be "FMRL_Nowhere | FMRL_Nowhere | FMRL_ArgumentPointees_OR_FMRL_InaccessibleMem | FMRL_Anywhere".
Do you think this is reasonable? Anything better?
Anyway I assume it should be discussed on dev list...

In D117095#3236981, @ebrevnov wrote:

In D117095#3236854, @nikic wrote:

Pretty sure this is dead code and the generic code already takes care of it. Your tests also pass for me without this patch.

For some reason generic code hadn't handled my original test case. Ok, let me check why... probably we should make generic code to be more generic :-)

In D117095#3236854, @nikic wrote:

Pretty sure this is dead code and the generic code already takes care of it. Your tests also pass for me without this patch.

Updated

Renamed

! In D102090#3174187, @rnk wrote:
In any case, -Bsymbolic is not a good default for the project because of issues like this.

I see. Thank you.

In D102090#3172538, @MaskRay wrote:

To add on rnk's comment (deduplication of vague linkage data which may be included in multiple shared objects), another big reason is -Bsymbolic data does not work with copy relocations. -fno-pic programs generally cannot avoid copy relocations (except mips; mips did this thing correctly).

Do you (by "doesn't work") mean if executable refers to a global defined in LLVM.*so then each of them will use their own copy?

In D102090#3169726, @rnk wrote:

In D102090#3169439, @ebrevnov wrote:

While -Bsymbolic-funtions brings nice performance improvements it also changes symbol resolution order. That means we effectively disabled preemption for functions and all references from inside libLLVM*.so will be resolved locally. But references to global data can still be interposed by external definitions. Do I understand correctly that main argument against using -Bsymbolic is potential issue with equality comparison of address of global? Or anything else?

I think it has less to do with the uniqueness of the addresses and more to do with the deduplication of the global storage. If you have an inline C++ global variable present in LLVM's headers (think a static data member of a class template instantiation), things go wrong quickly if there are two copies of this global, one in LLVM, and the other in the user's binary. Updates from one DSO will not be visible in the other.

I don't see what's wrong here (maybe you have specific example). IMHO, quite opposite, LLVM will consistently use it's own instance while user's binary it's own as if there were two globals with different names in the first place. I have exactly that real life case where LLVM's function (coming from libLLVM*.so since functions are not interposed) refers to a global from the user's binary with the same name by completely different semantics.

While -Bsymbolic-funtions brings nice performance improvements it also changes symbol resolution order. That means we effectively disabled preemption for functions and all references from inside libLLVM*.so will be resolved locally. But references to global data can still be interposed by external definitions. Do I understand correctly that main argument against using -Bsymbolic is potential issue with equality comparison of address of global? Or anything else?

Updated

Please note we still vectorize epilog with VF 8 while the remainder number of iterations is known to be 1. That should be addressed separately though..

Merge with D112312

I'm totally fine if you commit changes by yourself. I will be able to get to it early next week only.

LGTM.
PS: I think it should be possible to create targeted test case by hands though.

It seems that the new heuristic is applied not only to loops in rotated form. For example "test_cold_loop" or "test13".
In general we should decrease probability to take particular back edge proportionally to number of added exits. For example if number of exits equal to number of predicted iterations then each particular exit should be 50/50. But this is not supported yet anyway..

The fix is sent for review https://reviews.llvm.org/D112060

There was another infinite loop example attached to the commit message for this patch - https://reviews.llvm.org/rG83d134c3c4222e8b8d3d90c099f749a3b3abc8e0
I'm not sure if that is the same root cause, but I don't think there was any reply to that failure.

In D109368#3028466, @fhahn wrote:

That would be good, but unfortunately I think the epilogue vectorizer instantiation only has access to an ElementCount for now :( Can the threaded through as follow-up

There is no cost associated with runtime checks for epilogue vectorization. Thus we should simply initialize 'MinProfitableTripCount' with 'unset' value in Epilogue Vectorizer.

In D109368#3022623, @fhahn wrote:

I am not sure what the trade-offs are to adding more VF independent cost-modeling there yet (drawbacks are adding even more global state to the cost model, increasing complexity).

Right, while cost of runtime-checks (at the moment) is VF independent we can decide not to do it inside cost model. Though conceptually it looks much more solid design when CostModel is responsible for the cost based decisions. And total code complexity will be even less than doing cost modeling in different places.

Generally LGTM (except few mentioned nits)

In D109368#3022623, @fhahn wrote:

Given the large update I did not adjust it yet on D109443. I'd suggest to discuss moving the code into the cost-model separately in your patches.

Sure let's discuss move to cost model separately. Having sad that first four patches in the series (including D109443) are general improvements independent of the change of cost model itself.
Of cause, there is no strong dependency and D109443 should not block this one.

In D109443#3022641, @fhahn wrote:

Also this facilitates future development.

Could you provide a few more details on this, i.e. what the drawbacks of explicit initialization are?

Honestly, I find current API a bit confusing. I see the following drawbacks in priority:

1. Current API is a potential source of future errors since it is easy to misplace a call to Create() or miss it completely. There is no easy way to detect the mistake.
2. Negatively affects end user experience if the existing API.
3. Requires code changes which could be avoided otherwise.