Page MenuHomePhabricator

ebrevnov (Evgeniy)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 23 2018, 4:17 PM (232 w, 14 h)

Recent Activity

Apr 5 2022

ebrevnov added a comment to D118440: New regression test against expandMemCpyAsLoop utility.

@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 :-)

Apr 5 2022, 3:42 AM · Restricted Project, Restricted Project
ebrevnov reopened D118440: New regression test against expandMemCpyAsLoop utility.
Apr 5 2022, 3:23 AM · Restricted Project, Restricted Project

Apr 4 2022

ebrevnov added a comment to D118440: New regression test against expandMemCpyAsLoop utility.

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

Apr 4 2022, 11:23 PM · Restricted Project, Restricted Project
ebrevnov updated the diff for D118443: Add support for atomic memory copy lowering.

Typo fixed Residal->Residual

Apr 4 2022, 11:20 PM · Restricted Project, Restricted Project
ebrevnov added inline comments to D118443: Add support for atomic memory copy lowering.
Apr 4 2022, 11:18 PM · Restricted Project, Restricted Project
ebrevnov updated the diff for D118443: Add support for atomic memory copy lowering.

Rebase + formatting

Apr 4 2022, 11:17 PM · Restricted Project, Restricted Project

Mar 30 2022

ebrevnov added a comment to D118440: New regression test against expandMemCpyAsLoop utility.

Looks good now?

Mar 30 2022, 9:16 PM · Restricted Project, Restricted Project

Mar 29 2022

ebrevnov updated the diff for D118443: Add support for atomic memory copy lowering.

Rebase

Mar 29 2022, 1:09 AM · Restricted Project, Restricted Project
ebrevnov updated the diff for D118441: Preserve aliasing info during memory intrinsics lowering.

Update

Mar 29 2022, 12:40 AM · Restricted Project, Restricted Project

Mar 28 2022

ebrevnov updated the diff for D118440: New regression test against expandMemCpyAsLoop utility.

Updated

Mar 28 2022, 11:08 PM · Restricted Project, Restricted Project
ebrevnov added inline comments to D118440: New regression test against expandMemCpyAsLoop utility.
Mar 28 2022, 9:29 PM · Restricted Project, Restricted Project
ebrevnov added inline comments to D118441: Preserve aliasing info during memory intrinsics lowering.
Mar 28 2022, 9:25 PM · Restricted Project, Restricted Project

Mar 15 2022

ebrevnov added a comment to rGcbaac1473403: [LV] Remove induction recipes only used outside vector loop..

The latest one (4a0481e981b653153fd07db9cbee1197ecd388d3 ) helps. Thanks!

Mar 15 2022, 10:06 PM
ebrevnov added a comment to rGcbaac1473403: [LV] Remove induction recipes only used outside vector loop..

Thanks, I will check tomorrow.

Mar 15 2022, 9:17 AM
ebrevnov added a comment to rGcbaac1473403: [LV] Remove induction recipes only used outside vector loop..

Could you point out to a fixing commit?

Mar 15 2022, 4:27 AM
ebrevnov added a comment to rGcbaac1473403: [LV] Remove induction recipes only used outside vector loop..

Hi Florian,

Mar 15 2022, 3:57 AM

Feb 25 2022

ebrevnov added a reviewer for D120552: [SLP] "Normal" instructions should not go between PHI and Lading pad: ABataev.
Feb 25 2022, 3:15 AM · Restricted Project
ebrevnov requested review of D120552: [SLP] "Normal" instructions should not go between PHI and Lading pad.
Feb 25 2022, 3:15 AM · Restricted Project

Feb 14 2022

ebrevnov added a comment to D90095: [DSE] Enable MSSA DSE to optimize across PHIs.

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 :-)

Feb 14 2022, 5:08 AM · Restricted Project
ebrevnov added a comment to D118441: Preserve aliasing info during memory intrinsics lowering.

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

Feb 14 2022, 4:09 AM · Restricted Project, Restricted Project
ebrevnov added a comment to D118441: Preserve aliasing info during memory intrinsics lowering.

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

Feb 14 2022, 3:44 AM · Restricted Project, Restricted Project

Feb 3 2022

ebrevnov updated the diff for D118443: Add support for atomic memory copy lowering.

Rebase

Feb 3 2022, 10:31 PM · Restricted Project, Restricted Project
ebrevnov updated the diff for D118441: Preserve aliasing info during memory intrinsics lowering.

Fixed according to comments

Feb 3 2022, 10:30 PM · Restricted Project, Restricted Project
ebrevnov updated the diff for D118440: New regression test against expandMemCpyAsLoop utility.

Updated

Feb 3 2022, 10:28 PM · Restricted Project, Restricted Project
ebrevnov added a comment to D118441: Preserve aliasing info during memory intrinsics lowering.

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.

Feb 3 2022, 1:01 AM · Restricted Project, Restricted Project

Jan 31 2022

ebrevnov added a comment to D118441: Preserve aliasing info during memory intrinsics lowering.

Needs some lit tests that show the metadata after expansion

Jan 31 2022, 8:02 PM · Restricted Project, Restricted Project
ebrevnov added inline comments to D118443: Add support for atomic memory copy lowering.
Jan 31 2022, 5:03 AM · Restricted Project, Restricted Project

Jan 28 2022

ebrevnov updated the summary of D118443: Add support for atomic memory copy lowering.
Jan 28 2022, 2:05 AM · Restricted Project, Restricted Project
ebrevnov updated the diff for D118443: Add support for atomic memory copy lowering.

Source and destionation of atomic memcopy may not overlap by sepcification

Jan 28 2022, 2:00 AM · Restricted Project, Restricted Project
ebrevnov updated the summary of D118441: Preserve aliasing info during memory intrinsics lowering.
Jan 28 2022, 1:57 AM · Restricted Project, Restricted Project
ebrevnov updated the summary of D118440: New regression test against expandMemCpyAsLoop utility.
Jan 28 2022, 1:44 AM · Restricted Project, Restricted Project
ebrevnov requested review of D118443: Add support for atomic memory copy lowering.
Jan 28 2022, 1:40 AM · Restricted Project, Restricted Project
ebrevnov requested review of D118441: Preserve aliasing info during memory intrinsics lowering.
Jan 28 2022, 1:15 AM · Restricted Project, Restricted Project
ebrevnov requested review of D118440: New regression test against expandMemCpyAsLoop utility.
Jan 28 2022, 12:50 AM · Restricted Project, Restricted Project

Jan 25 2022

ebrevnov added a comment to D118066: [SimplifyCFG] Don't speculatively execute preductably-taken block.

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.

Jan 25 2022, 9:47 PM · Restricted Project, Restricted Project

Jan 24 2022

ebrevnov added inline comments to D115710: [LV][NFC] Update test checks using utils/update_test_checks.py.
Jan 24 2022, 5:07 AM · Restricted Project
ebrevnov added a comment to D115712: [LV] Make sure prefer-predicate-over-epilogue works for short TC loops.

Ping

Jan 24 2022, 4:59 AM · Restricted Project
ebrevnov added a comment to D115711: [LV][NFC] New test case to check prefer-predicate-over-epilogue for short TC loops.

Ping

Jan 24 2022, 4:56 AM · Restricted Project
ebrevnov added a comment to D115710: [LV][NFC] Update test checks using utils/update_test_checks.py.

Ping

Jan 24 2022, 4:56 AM · Restricted Project
ebrevnov added a comment to D117095: [BasicAA] Add support for memmove intrinsic.

@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.

Jan 24 2022, 4:39 AM · Restricted Project
ebrevnov updated the diff for D117095: [BasicAA] Add support for memmove intrinsic.

Fix comment

Jan 24 2022, 4:15 AM · Restricted Project
ebrevnov added reviewers for D118033: [AA] Refine ModRefInfo for llvm.memcpy.* in presence of operand bundles: reames, fhahn, nikic.
Jan 24 2022, 4:12 AM · Restricted Project
ebrevnov requested review of D118033: [AA] Refine ModRefInfo for llvm.memcpy.* in presence of operand bundles.
Jan 24 2022, 4:11 AM · Restricted Project
ebrevnov updated the diff for D117095: [BasicAA] Add support for memmove intrinsic.
Jan 24 2022, 4:01 AM · Restricted Project
ebrevnov added reviewers for D118031: [NFC] New test case for BasicAA and memcy/memmove with deopt: reames, fhahn, nikic.
Jan 24 2022, 4:00 AM · Restricted Project
ebrevnov requested review of D118031: [NFC] New test case for BasicAA and memcy/memmove with deopt.
Jan 24 2022, 3:58 AM · Restricted Project

Jan 14 2022

ebrevnov added a comment to D117095: [BasicAA] Add support for memmove intrinsic.

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...

Jan 14 2022, 6:28 AM · Restricted Project

Jan 12 2022

ebrevnov added a comment to D117095: [BasicAA] Add support for memmove intrinsic.

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 :-)

Jan 12 2022, 6:01 AM · Restricted Project
ebrevnov added a comment to D117095: [BasicAA] Add support for memmove intrinsic.

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

Jan 12 2022, 2:22 AM · Restricted Project
ebrevnov added reviewers for D117095: [BasicAA] Add support for memmove intrinsic: reames, fhahn, nikic.
Jan 12 2022, 1:37 AM · Restricted Project
ebrevnov requested review of D117095: [BasicAA] Add support for memmove intrinsic.
Jan 12 2022, 1:34 AM · Restricted Project

Dec 14 2021

ebrevnov updated the summary of D115713: [LV] Don't apply "TinyTripCountVectorThreshold" for loops with compile time known TC..
Dec 14 2021, 2:42 AM · Restricted Project
ebrevnov updated the summary of D115712: [LV] Make sure prefer-predicate-over-epilogue works for short TC loops.
Dec 14 2021, 1:53 AM · Restricted Project
ebrevnov updated the summary of D115711: [LV][NFC] New test case to check prefer-predicate-over-epilogue for short TC loops.
Dec 14 2021, 1:41 AM · Restricted Project
ebrevnov updated the summary of D115710: [LV][NFC] Update test checks using utils/update_test_checks.py.
Dec 14 2021, 1:38 AM · Restricted Project
ebrevnov requested review of D115713: [LV] Don't apply "TinyTripCountVectorThreshold" for loops with compile time known TC..
Dec 14 2021, 1:37 AM · Restricted Project
ebrevnov requested review of D115712: [LV] Make sure prefer-predicate-over-epilogue works for short TC loops.
Dec 14 2021, 1:37 AM · Restricted Project
ebrevnov requested review of D115711: [LV][NFC] New test case to check prefer-predicate-over-epilogue for short TC loops.
Dec 14 2021, 1:36 AM · Restricted Project
ebrevnov requested review of D115710: [LV][NFC] Update test checks using utils/update_test_checks.py.
Dec 14 2021, 1:36 AM · Restricted Project

Dec 13 2021

ebrevnov added inline comments to D114528: [LV] Make sure VF doesn't exceed compile time known TC.
Dec 13 2021, 3:15 AM · Restricted Project

Dec 10 2021

ebrevnov updated the diff for D114528: [LV] Make sure VF doesn't exceed compile time known TC.

Updated

Dec 10 2021, 12:51 AM · Restricted Project
ebrevnov added a reviewer for D114526: [LV][NFC] New test case for compile time known trip count (TC): fhahn.
Dec 10 2021, 12:33 AM · Restricted Project
ebrevnov updated the diff for D114526: [LV][NFC] New test case for compile time known trip count (TC).

Renamed

Dec 10 2021, 12:09 AM · Restricted Project

Dec 9 2021

ebrevnov added a comment to D102090: [CMake][ELF] Link libLLVM.so and libclang-cpp.so with -Bsymbolic-functions.

! 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.

Dec 9 2021, 1:05 AM · Restricted Project, Restricted Project

Dec 6 2021

ebrevnov added a comment to D102090: [CMake][ELF] Link libLLVM.so and libclang-cpp.so with -Bsymbolic-functions.

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?

Dec 6 2021, 1:10 AM · Restricted Project, Restricted Project
ebrevnov added a comment to D102090: [CMake][ELF] Link libLLVM.so and libclang-cpp.so with -Bsymbolic-functions.

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.

Dec 6 2021, 12:21 AM · Restricted Project, Restricted Project

Dec 3 2021

ebrevnov added inline comments to D114526: [LV][NFC] New test case for compile time known trip count (TC).
Dec 3 2021, 3:48 AM · Restricted Project
ebrevnov added inline comments to D114528: [LV] Make sure VF doesn't exceed compile time known TC.
Dec 3 2021, 3:46 AM · Restricted Project
ebrevnov added a comment to D102090: [CMake][ELF] Link libLLVM.so and libclang-cpp.so with -Bsymbolic-functions.

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?

Dec 3 2021, 3:13 AM · Restricted Project, Restricted Project

Nov 25 2021

ebrevnov updated the diff for D114528: [LV] Make sure VF doesn't exceed compile time known TC.

Updated

Nov 25 2021, 9:42 PM · Restricted Project
ebrevnov added inline comments to D114528: [LV] Make sure VF doesn't exceed compile time known TC.
Nov 25 2021, 9:36 PM · Restricted Project

Nov 24 2021

ebrevnov added a comment to D114528: [LV] Make sure VF doesn't exceed compile time known TC.

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..

Nov 24 2021, 5:23 AM · Restricted Project
ebrevnov updated the summary of D114528: [LV] Make sure VF doesn't exceed compile time known TC.
Nov 24 2021, 5:22 AM · Restricted Project
ebrevnov requested review of D114528: [LV] Make sure VF doesn't exceed compile time known TC.
Nov 24 2021, 5:08 AM · Restricted Project
ebrevnov requested review of D114526: [LV][NFC] New test case for compile time known trip count (TC).
Nov 24 2021, 4:53 AM · Restricted Project

Nov 22 2021

ebrevnov updated the diff for D105098: [DSE][NFC] Introduce "doesn't overwrite" return code for isOverwrite.

Merge with D112312

Nov 22 2021, 4:55 AM · Restricted Project

Nov 19 2021

ebrevnov added a comment to D112312: [DSE] Add OR_None (not for commit).

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

Nov 19 2021, 5:27 AM · Restricted Project

Nov 15 2021

ebrevnov added inline comments to D112312: [DSE] Add OR_None (not for commit).
Nov 15 2021, 12:59 AM · Restricted Project

Nov 11 2021

ebrevnov added inline comments to D112312: [DSE] Add OR_None (not for commit).
Nov 11 2021, 8:44 PM · Restricted Project

Nov 7 2021

ebrevnov accepted D113344: [BPI] Push exit block rather than exiting ones in getSccExitBlocks.

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

Nov 7 2021, 11:10 PM · Restricted Project
ebrevnov added a comment to D113345: [WIP][BPI] `calcEstimatedHeuristics()`: symmetrically with loop exiting edge, scale loop enter edge weight by trip count.

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..

Nov 7 2021, 11:01 PM · Restricted Project

Oct 20 2021

ebrevnov added inline comments to D112060: [NARY-REASSOCIATE] Fix infinite recursion optimizing min\max.
Oct 20 2021, 3:07 AM · Restricted Project
ebrevnov added reviewers for D112128: [NARY-REASSOCIATE][NFC] Simplify min/max handling: nikic, mkazantsev, spatel.
Oct 20 2021, 3:06 AM · Restricted Project
ebrevnov requested review of D112128: [NARY-REASSOCIATE][NFC] Simplify min/max handling.
Oct 20 2021, 3:04 AM · Restricted Project

Oct 19 2021

ebrevnov added inline comments to D112060: [NARY-REASSOCIATE] Fix infinite recursion optimizing min\max.
Oct 19 2021, 10:53 PM · Restricted Project
ebrevnov added inline comments to D111668: [LoopPredication] Calculate profitability without BPI.
Oct 19 2021, 10:30 PM · Restricted Project
ebrevnov added a comment to D88287: [NARY-REASSOCIATE] Support reassociation of min/max.

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

Oct 19 2021, 3:19 AM · Restricted Project
ebrevnov added reviewers for D112060: [NARY-REASSOCIATE] Fix infinite recursion optimizing min\max: mkazantsev, spatel.
Oct 19 2021, 3:17 AM · Restricted Project
ebrevnov requested review of D112060: [NARY-REASSOCIATE] Fix infinite recursion optimizing min\max.
Oct 19 2021, 3:15 AM · Restricted Project
ebrevnov added inline comments to D111668: [LoopPredication] Calculate profitability without BPI.
Oct 19 2021, 1:25 AM · Restricted Project

Oct 18 2021

ebrevnov added a comment to D88287: [NARY-REASSOCIATE] Support reassociation of min/max.

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.

Oct 18 2021, 10:40 PM · Restricted Project

Oct 1 2021

ebrevnov added inline comments to D75981: [LV] Create RT checks once VF/IC are selected, track scalar cost..
Oct 1 2021, 12:14 AM · Restricted Project, Restricted Project
ebrevnov added inline comments to D109368: [LV] Vectorize cases with larger number of RT checks, execute only if profitable..
Oct 1 2021, 12:04 AM · Restricted Project, Restricted Project

Sep 30 2021

ebrevnov added a comment to D109368: [LV] Vectorize cases with larger number of RT checks, execute only if profitable..

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.

Sep 30 2021, 11:58 PM · Restricted Project, Restricted Project

Sep 27 2021

ebrevnov added a comment to D109368: [LV] Vectorize cases with larger number of RT checks, execute only if profitable..

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.

Sep 27 2021, 2:12 AM · Restricted Project, Restricted Project
ebrevnov accepted D109368: [LV] Vectorize cases with larger number of RT checks, execute only if profitable..

Generally LGTM (except few mentioned nits)

Sep 27 2021, 12:38 AM · Restricted Project, Restricted Project
ebrevnov added inline comments to D109368: [LV] Vectorize cases with larger number of RT checks, execute only if profitable..
Sep 27 2021, 12:33 AM · Restricted Project, Restricted Project
ebrevnov added a comment to D109368: [LV] Vectorize cases with larger number of RT checks, execute only if profitable..

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.

Sep 27 2021, 12:11 AM · Restricted Project, Restricted Project
ebrevnov added inline comments to D109443: [LV] Lazy creation of runtime checks.
Sep 27 2021, 12:00 AM · Restricted Project

Sep 26 2021

ebrevnov added a comment to D109443: [LV] Lazy creation of runtime checks.

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.
Sep 26 2021, 11:49 PM · Restricted Project