LGTM but please get a second approval before submitting

LGTM but please get a second approval before submitting

LGTM but please get a second approval before submitting.

Would it be possible to test this as-is, or are other changes necessary to meaningfully exercise this code?

LGTM

LGTM

LGTM

Kind ping

LGTM

In D125701#3537479, @mehdi_amini wrote:

Can you clarify the motivation for this?

Ping

LGTM but please get a second approval before submitting.

In D124804#3526729, @beanz wrote:

libObject uses MemoryBufferRefs which play back and forth with StringRef, and while it has been pointed out over and over again that many of StringRef’s methods should probably be on MemoryBufferRef because they are more broadly useful, it is the way it is.
I think ArrayRef<char> is probably the least common pattern for representing arbitrary data buffers in LLVM, and given the needs of libObject to operate with MemoryBufferRef I think this code is best to operate on StringRefs.

On high level, would it be possible to use an existing class like llvm::BinaryStreamReader instead that most of this data reading machinery?

In D123481#3505002, @GMNGeoffrey wrote:

In D123481#3442718, @kuhar wrote:

+1, I had to patch this locally in my project for exactly the reasons you mentioned

I don't understand what you mean about having to patch it locally in a different project. bazelrc files are only applicable to the project itself, not anything that uses it as a dependency.

LGTM but please get another one before submitting

LGTM but please get another one before submitting.

LGTM modulo nits. Thanks for the fixes!

+1, I had to patch this locally in my project for exactly the reasons you mentioned

LGTM

Looks good to me overall, I just left some local comments. Please take my writing suggestions with a pinch of salt, English is my second language.

LGTM, thanks for the fixes.

Could you also add some tests that make sure this can be constant-evaluated? (e.g., with static_assert)

In D122079#3401001, @beanz wrote:

In D122079#3396156, @kuhar wrote:

A few thoughts:

1. Is the requirement for this to be constant folded by the optimizer?
2. This could be made constexpr if we wanted.
3. The function name suggests this handles enums only but it works with any copybale & comparable type. I think we should either restrict it to enum types or make the name more general.
4. Since this function is not constexpr now, how about we added an overload to is_contained accepting an initializer list as the second element, e.g.: is_constained(MyEnum::A, {MyEnum::A, MyEnum::B, MyEnum::C})?

I'm not too sure about the semantics either. I think it might be easier to think about if we tie this back to regular graphs without multi-edges.

LGTM but please get at least one additional approval before submitting

In D119061#3312571, @xbolva00 wrote:

Do you plan to also add inline and flatten?

You mean always_inline? Yes, after noinline. The flatten call site attribute - theoretically why not, but it needs to be reworked in LLVM (like always_inline_recursively) before any patch like this one.

Thanks for working on this, @xbolva00! I don't know this part of the codebase, so won't comment on the patch itself. Just a few questions/suggestions:

LGTM

In D117537#3271928, @sebastian-ne wrote:

@phosek, this patch fixes a regression that was introduced with D116521.
Could we fix this regression first with a simple patch that does not risk to be reverted again and do further refactorings afterwards?
Our downstream gcc build is broken and we’d like to re-enable it rather sooner than later.

LGTM

This change is NFC but can improve the runtime of the compiler dramatically in some pathological cases (where the pass was pushing a lot (several thousands) of small updates (less than 6)).
For instance on the motivating example we went from 300+ sec to less than a second.

I think it would be good to send the DomTree documentation changes separately as they are a clear improvement regardless of whether this patch goes in or not.

In D107378#3112933, @vvereschaka wrote:

Hi @kuhar,

looks like these changes are reason of bunch of build warnings on the windows builders (VC). I see more than 1000 additional warnings such as

C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\include\llvm/ADT/Sequence.h(119): warning C4309: 'static_cast': truncation of constant value
C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\include\llvm/ADT/Sequence.h(120): warning C4309: 'static_cast': truncation of constant value
C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\include\llvm/ADT/Sequence.h(119): warning C4309: 'static_cast': truncation of constant value
C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\include\llvm/ADT/Sequence.h(120): warning C4309: 'static_cast': truncation of constant value

starting at this build https://lab.llvm.org/buildbot/#/builders/60/builds/5295

would you take care of them?
Thank you.