- User Since
- Nov 12 2014, 1:58 PM (165 w, 6 d)
I think this is fine, but I'll defer to Zachary.
Sun, Jan 14
Fri, Jan 12
I might consider holding my nose, if this restores something that was there.
I think the other two passes we have for doing sinking aren't currently enabled (GVNSink and Sink.cpp), although unless somebody puts effort in them this will always be a chicken-egg problem.
I'm not sure I'll have the time to review this patch carefully, I don't want to put you on the hook, but if you can consider an alternative, that would be great.
If you look at the original review you'll notice that I was fundamentally opposed to the change, see e.g. https://reviews.llvm.org/D38566#926530
(FWIW, it doesn't matter where we move sinking/hoisting there will be always some case that we can't get properly. We, of course should prioritize for the common case, at least IMHO).
(As I already previously advocated), instcombine & simplifyCFG shouldn't do any hoisting/sinking whatsoever, but I only had limited energy to fight this battle.
+1, we have a dedicated sinking pass. Not blaming on you, but unfortunately SimplifyCFG is becoming a kitchen sink, let's try to keep it at least half-sane :)
Have you considered checking in the final artifacts (bitcode), maybe with a short description on how to craft them in case we need to regenerate?
I don't mind the direction but this needs a test.
Thu, Jan 11
Wed, Jan 10
This LGTM but please wait for a second opinion.
Go ahead if you still see this warning.
Tue, Jan 9
No objections from me. Thanks.
Mon, Jan 8
I don't feel qualified enough to say whether this can go in, somebody with fast-math experience should comment.
hmm, native makes this a pain. Probably not worth the trouble.
This change looks fine, but can you add a unit test (if possible)?
I think the correct fix here is that suggested by @dblaikie. Eventually, FWIW, this header might disappear entirely and we could replace the private lldb implementation with the LLVM one (feel free to take this if you're interested).
For reference. this is what GCC generates (although it's unclear whether it's a good idea to follow them)
@scanon should sign off this.
@djtodoro can you commit this or you need somebody to do it on your behalf?
LGTM modulo minor.
Sun, Jan 7
I don't mind this change, but before accepting, can you please show the other bits? (i.e. how do you plan to use these?)
Fri, Jan 5
LGTM, thanks Vedant!
Thu, Jan 4
Tue, Jan 2
Nice to see this for MBB, +1 modulo two minors.
I wonder whether you can use something like yaml2obj to craft the object? (or, assuming it's valid, llvm-mc)?
That would improve the readability a lot IMHO.
If not, can you at least add comments to the test (e.g. source file + compiler/linker version etc..) in case we need to regenerate this in the future?
LGTM, thanks Anna. (sorry for the delay, I was in Europe for the end of the year).
I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?
Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)
Something else you may want to keep in mind is that these transformations may introduce dramatic rounding errors, so they need some thoughts/analysis before they can go in (cc: @scanon), even under -ffast-math.
Meta point: It's unclear to me whether this is something profitable to implement.
Sure, you can probably take a textbook and implement all the possible identities written there, but, what's the point?
Sun, Dec 31
Fri, Dec 29
Can you add a unittest to show this issue?
Thu, Dec 28
That said, this one is probably one we really want. I skimmed the code quickly and I think it's correct, but please wait for somebody else to take a look
I'm slightly worried about all this bunch of missing instcombines added, as InstCombine is already really really slow.
Yes, this sounds like classic iterator invalidation. This is probably safe.
Tue, Dec 26
@mzolotukhin may want to comment on this one before it goes in as he's spending large part of his time doing compile time work. Please wait for his opinion.
It's not entirely clear to me why you need another reassociate pass.
We should really try to share as much code as possible with the existing one instead of adding maintenance burden.
Sun, Dec 24
Sat, Dec 23
Sorry, I clearly misread your previous diff. LGTM.
Fri, Dec 22
LGTM with one comment addressed. Thanks.
I'm across the pond fo the rest of the year, sorry if communication is a little slow :)
Thu, Dec 21
I think this is on the right track, but, can you please simplify the test case so I can read it better :) ?
Can you please add a testcase, Petr?
Indeed, this needs to be tested.
Wed, Dec 20
I don't have access to my checkout here, but I'm fairly sure we do something similar in SimplifyLibCalls.
I understand many of the pattern proposed will be duplicated, which is, not ideal.
Also, I'm pretty sure @scanon once taught me (when I implemented a similar transformation in simplifylibcalls) that these can underflow/overflow pretty dramatically, so, shouldn't these only be enabled under -ffast-math ?
Mon, Dec 18
Dec 17 2017
With that in mind:
- You may want to consider adding context to your patch, i.e. via git diff -U500000 &> blah.patch
- You may want not jump to conclusions so quickly. I'm pretty sure people care about your contributions, it's just that nobody got around to review your patch.
@ngie, Vedant can probably take a look.
Sometimes code reviews get lost, and it's generally good to ping people here or on IRC.