- User Since
- Jan 25 2017, 1:38 AM (120 w, 6 d)
Tue, Apr 23
Can you please say what is the reason to move the function to CallInst only but not for CallBase?
Mon, Apr 22
Sun, Apr 21
Handled comment, please review.
Apr 18 2019
Apr 17 2019
Apr 15 2019
Apr 14 2019
Apr 4 2019
Apr 3 2019
Apr 2 2019
Mar 28 2019
Thank you for reverting the patch, I'll take a look into this dependency. I'll ping you if I have a trouble in reproducing the crash.
Mar 27 2019
Mar 26 2019
Fedor's comments handled.
Mar 10 2019
Jan 24 2019
Abandon the review.
Jan 15 2019
Jan 13 2019
Philip, I'm not sure that I completely follow your comment about spirit of the tests but I've added a coverage for size/alignment.
I hope it will be enough.
Jan 11 2019
Jan 10 2019
Dec 27 2018
Ping, any progress on this?
Dec 18 2018
I've verified that this patch fixes the original problem.
Dec 17 2018
I was able to create a pure LLVM reproducer for this issue and filed a bug https://bugs.llvm.org/show_bug.cgi?id=40061.
Could you please take a look into it?
Dec 11 2018
Hello @wmi, in our local fuzzing testing we've got the following assert:
src/include/llvm/ADT/IntervalMap.h:630: unsigned int llvm::IntervalMapImpl::LeafNode<KeyT, ValT, N, Traits>::insertFrom(unsigned int&, unsigned int, KeyT, KeyT, ValT) [with KeyT = llvm::SlotIndex; ValT = unsigned int; unsigned int N = 9u; Traits = llvm::IntervalMapInfo<llvm::SlotIndex>]: Assertion `!Traits::stopLess(b, a) && "Invalid interval"' failed.
Dec 6 2018
Nov 29 2018
abandon in favor of D54932.
Nov 28 2018
Absolutely agreed. It was just a sid comment.
(As an aside, other than a compiler-crash during vectorization (due to a deref of a null pointer), this is my first experience in the llvm vectorizer. So I'm not familiar with how to go about adding the improvement you suggested.
I'm not sure whether you use a custom pipeline or not. But my point was that code came to vectorization pass seems strange because trivial loop unswitch optimization (provided by LoopUnswitch pass) could hoist the invariant check. By some reason it did not happen.
It's experience I'd like to gain, so I'll consider it as possible future work. But to make sure I'm not misleading you, I don't have time to work on this in the immediate future.)
Not a problem at all.
This is invalid patch.
LGTM. If Max or Sanjoy does not object in a, say, one day, feel free to land.
Nov 27 2018
Agreed, I missed comments in hurry. Will update a patch...
Nov 26 2018
FYI: in parallel there is an idea how to simplify algorithm significantly. I'll try to implement it...
Nov 22 2018
Feel free to mention @dmgreen in description who found this bug.
Thank you, Bjorn.
Nov 20 2018
Hi Bjorn, thank you for looking into this patch.
Nov 19 2018
Basing on https://bugs.llvm.org/show_bug.cgi?id=23603 I think this patch is incorrect.
Nov 14 2018
Nov 12 2018
Nov 11 2018
LGTM as well
Nov 8 2018
Ok, I was wrong Sink pass iterates. If I add a support for invariant.load it can sink it but it definitely cannot sink it through diamond or loop. In parallel I'm going to take a look deeper whether I can handle my case in Sink pass...
Handled comments. Fow a while I'll double check Sink.cpp...
Hi @arsenm, could you please take a look into this patch? If I turn the option on I see some AMDGPU test failures. I'm not an expert in AMDGPU asm/arch but it seems that 4 invariant loads are added in the beginning of pipeline in entry block and I hoist one of them to colder block. As a result as I understand instead of loading of two load at once this "double-load" is split into two loads. I'm not sure that it is a beneficial for this platform. So I wonder whether there is a easy way to prohibit optimization for such cases for this platform?
Even if there is no easy way to do that I'd like to land it with option off by default.
Nov 7 2018
In general I think it is what we can go/start with.
This formula seems to be a bit pessimistic. Specifically not every candidate creates sibling loops. Only if all successors is in loop or there are sub-loops before unswitch candidate. So applying the power multiplier for this candidates seems unreasonable.
Also it is possible that several unswitch candidates can be done at one shot (the same condition), in this case the number of siblings will not grow so fast.
But these improvements can be done as a follow-up if needed.
Just a note, there is a specific usage of this data structure.
Phi nodes are inserted in the initial step without any removals and then only removals are used.
Nov 6 2018
LGTM, please wait a bit and if Bjorn does not object, feel free to land.
Nov 5 2018
Also Max, please consider split this patch into two ones. One just NFC patch with analysis and update tests to check the output of dump and then patches which does modifications.