In rG5bb38e84d3d0420e7159ffc34e3dd33ab5088b8e#986266, @aqjune wrote:

I made a fix to keep the main branch in a good state, but not sure this is a real solution because there might be a hidden problem. I'm investigating it.

Hi! This commit causes miscompilation.
Here's the test:

define i32 @foo(i32* %arg1, i1 %arg2, i1* %arg3, i1* %arg4) {
bb:
  %tmp7 = icmp ne i1 %arg2, 0
  %tmp9 = xor i1 %tmp7, true
  %tmp10 = or i1 %tmp7, %tmp9
  br label %bb11

Hi Roman!
I don't know if this information will help you, but after c9dc627331992b01b962103fad813810cba00696 on internal testing we started hitting following assertion at very low rate

llvm/include/llvm/ADT/DenseMap.h:408 assert(!FoundVal && "Key already in new map?");

I checked it and it looks like if it compiles, everything should work just fine in presence of incomplete types.
LGTM

Apparently, it breaks MSVC buildbots too, but it does it in a way I can't comprehend: looks like after my change MSVC instantiates all methods of BlockFrequencyInfoImpl, including ones that weren't instantiated before and which are incorrect for <MachineBasicBlock> instantiation. I don't have access to MSVC, so there's no way I'll figure out what's wrong with it and fix it.

Following resubmission of this patch:

commit 8975aa6ea8172963d6532caa8ed2a6f6e0074a02
Author: Daniil Suchkov <suc-daniil@yandex.ru>
Date:   Thu Mar 5 18:32:50 2020 +0700

Hi! I had to revert this patch because following test results in SIGSEGV and passes with the patch reverted (you can find it at llvm/test/Transforms/LoopRotate/freeze-crash.ll):

; RUN: opt -loop-rotate -disable-output %s

Hi! Unfortunately I had to revert this patch, see my comment here: https://reviews.llvm.org/rG952ad4701cf0d8da79789f6b83ddaa386c60d535

Actually, this change will even help CheckBFIUnknownBlockQueries: since every missed update will result in a missing NodesMap entry, without exclusion of cases when new BB happens to be in the same address where some old BB was.

In D75341#1904797, @davidxl wrote:

I think we should explicitly fix the real problems that new blocks are not updated frequency information instead of just letting them get a known default value. Doing it later can make it harder to find those problems, right?

You mean we don't need to update BFI in non-asserting mode? It's not just for verification, it's required for correctness.

In D75341#1903988, @davidxl wrote:

This looks good, but it does have implications on memory consumption for BFI. To avoid that, gate the data structure change on #ifndef NDEBUG.

Got rid of all const_casts except one, which was moved into CallbackVH's constructor.

One thing I don't understand is why three out of four kinds of VHs perform the const_cast, not the ValueHandleBase. I think it's time to change that (and by doing so, allow CallbackVH accept const pointers too).
Given that three out of four VH kinds already perform const_cast inside, I think it makes sense to add one to CallbackVH too.

In D75341#1899307, @davidxl wrote:

Do you mean it does not work well for const pointers? There are existing uses of AssertingVH<const BasicBlock> or AssertingVH<const Value>?

In D75341#1898807, @fhahn wrote:

Alternatively I think it would be reasonable to expect the passes claiming to preserve BFI to notify BFI about deleted/added blocks, like we do for many other analysis. Do you have any idea of how much effort that would take?

In D75341#1898925, @davidxl wrote:

Can you make BlockKeyT to be AssertingVH<const BasicBlock> to avoid the const castings?

Given this comment: https://bugs.llvm.org/show_bug.cgi?id=44320#c3 there is a better solution than this patch.

In D70593#1759353, @efriedma wrote:

Are the changes to loop unswitch supposed to be in this patch?

In D70423#1756962, @fhahn wrote:

I might be missing something obvious, but is the motivation for adding this that the new pass manager does not use verifyAnalysis like the legacy pass manager? If that's the case, it would be great if you could clarify that in the title/description.

LGTM

LGTM

Removed support of non-inbounds pointers.

In D69369#1723838, @reames wrote:

I think you can write this test better as an lit test with strict validation. I assume SCEV has an expensive asserts mode - if not, we should add one - and thus this can be written as a lit test with an extra flag to opt.

ping?

@asbirlea could you please also take a look at the parent revision for this patch that adds the unit test (D69369)?
And if everything looks good, may I ask you to submit both patches? I don't have commit access yet.

Added a comment explaining how IsInBounds is set by stripAndAccumulateConstantOffsets.

In D68128#1709717, @spatel wrote:

I didn't step through the entire patch/tests, but the first few examples are flattened/simplified by -simplifycfg. Would it be better to look there to add the missing functionality? Is there a test in this patch that shows a case that simplifycfg can't already flatten?

Gentle ping?

Test checks updated.

Comment addressed: got rid of duplicating checks with different prefixes.

In D68128#1689376, @lebedev.ri wrote:

I see.
So we want to hoist identical gep+bitcast from 2 BB's into their closest dominating BB, more or less.
Do we have standalone hoisting pass?

In D68128#1685720, @lebedev.ri wrote:

Should this go elsewhere, SimplifyCFG?

Comments addressed: checks in tests are autogenerated, positive tests moved to a separate patch D68263, got rid of excessive use of auto keyword.

Addressed Serguei's comment. Serguei, could you please submit this patch? I don't have permissions to submit patches yet.

@skatkov, could you please submit this patch? I don't have permissions to submit patches yet.