- User Since
- Jan 25 2017, 1:38 AM (112 w, 1 d)
Sun, Mar 10
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.
Oct 15 2018
Sep 10 2018
Sep 9 2018
The same comment as in D51777, The semantics of the patch itself lgtm.
I see that IndVarsSimplify uses class field Changed. You propose to use Changed as return value. I just want to see the same approach through the whole IndVars...
Sep 7 2018
LGTM with follow-up handling of Changed. If you do not have cycles for handling Changed, please file a bug at least.
Sep 6 2018
is it better?
Sep 5 2018
Sep 3 2018
Sep 2 2018
Could you please add a test producing something with #2 and #3... I guess any loop pass for function with 2-3 loops should work.
Aug 20 2018
Verifier fix landed, can we proceed with this change?
Aug 19 2018
Aug 17 2018
Test is updated to be valid.
Aug 16 2018
ping. Can I do anything to proceed with this change?
Aug 12 2018
Aug 10 2018
Thank you! I've added two unit tests.
Thank you for your comment. Unfortunately till now I failed to create a test showing the benefit from this change in a pass (I tried). I can make one more try.
The problem I'm trying to solve is the following: in downstream pass I have a BitCast instruction which is actually originated from allocation but has a vector type gotten using gep + bitcast from allocation.
I'd like to know the fact that it is an underlying allocation but now utility function just ignore instructions with vector of pointers.
Aug 8 2018
Jul 1 2018
Jun 27 2018
Test, actually can be simplified bit if you do not mind.
Jun 4 2018
LGTM. If no one objects in 1-2 days, feel free to land.
Jun 3 2018
May 31 2018
Please take a look.