@kmitropoulou will take over with a new revision.

@kmitropoulou noted that the PHI of ops transformation in the test is completely valid. This test might be a nice addition with llvm.assume() intrinsic in the LIT test suite.

Inserting the cloned instruction ValueOp after I at https://github.com/llvm/llvm-project/blob/6954cb54252b50df95eea05c513e739b3ad2c8a0/llvm/lib/Transforms/Scalar/NewGVN.cpp#L2767 incorrectly deduces values in tests involving PHI of ops.

LGTM

Added a test for an unsized global compare

Fix assertion failure by adding a sized type check.

Sorry, I was occupied with other projects. I can fix this patch.

In D154307#4467219, @nikic wrote:

After @arsenm's change, InstSimplify should no longer work with instructions without parent. Can you explain how we end up working on an assume without parent in NewGVN? It's not really obvious to me.

Thanks for the review!

Restored tests in objectsize_basic.ll

Return unknown() for GV.hasExternalWeakLinkage(). Eliminate last two tests in objectsize_basic.ll

Modified tests

Added more tests as requested.

• Added a LowerConstantIntrinsic test as requested

In D152145#4407171, @arsenm wrote:

In D152145#4401053, @gandhi21299 wrote:

• Requested changes

I am not sure how to implement a test with EvalMode != Min.

My skimming suggests lowering an objectsize intrinsic call would work

• Requested changes

In D152145#4399093, @arsenm wrote:

Any chance this fixes issue #58617?

Added checks for NullIsUnknownSize and EvalMode to handle interposable cases.

• Compute object size of interposable gv, add test for it.

• Added !

I only found one place in the code where there may be structural changes in the CFG.

^

• Revert back to rev-1

Preserve TargetLibraryAnalysis and AssumptionAnalysis in all cases. If no changes are made by the pass, preserve all analyses. Otherwise preserve the two only.

• Suggested edits

• rebase against main

• Preserve CFGAnalyses if the function was modified (asserting no structural changes). Preserve all analyses otherwise.

• Right, I believe all analyses are preserved @arsenm

• Removed the inclusion of 'Instructions.h'

Many thanks for the code review. I will push this patch to main.

• Simplified code
• Made inttoptr_merge() target independent by inserting align 8 to store instructions and support from D75362 as mentioned

• clang-format and rebase

• Simplified OtherStoreIsMergeable as requested

• rebase and re-run premerge checks

• Brought back the break statement which was removed accidentally
• OtherStoreIsMergable returns a bool now, instead of a StoreInst *
• Corrected tests

• Use IRBuilder to create cast
• Eliminate InsertBitcast flag
• Fixed alignment test
• Removed redundant checks
• Eliminated temporary store

• Replaced bitwidth equivalence check with isBitOrNoopPointerCastable()
• Inserted hasSameSpecialState() check
• Added a test with different alignments
• Added ptrtoint test
• Created an AMDGPU-specific test for inttoptr. This test appears to add an iteration to instruction combining until it reaches a fixed point. That is, instcombine will assume that it will run indefinitely when instcombine-infinite-loop-threshold is set to 3 whereas the test passes when I change the threshold value to 4.

Thanks everyone for feedback to this review. I implemented a different approach in D150900, which is rather safer than this approach. I will abandon this revision as a result.

@nikic @nlopes Is there a way to reliably infer the type of alloca then? The purpose is to enable certain optimizations like merging multiple stores into a phi, etc.

• Simplified test further by eliminating addrspacecast

• Simplified @test7 in memcpy_alloca.ll

Internal CI passed.

In D149568#4311882, @foad wrote:

Should probably fix the other FIXME from 7c8b8063b66c7b936d41a0c4069c506669e13115 at the same time.

In D147424#4240712, @Joe_Nash wrote:

Since printing to errs() seems off the table, some other potential options:

• Add proper warning infrastructure to the disassembler

Done

LGTM. Please wait for a day before committing in case other reviewers have any comments.

• Applied clang-format

Submitted a new patch: https://reviews.llvm.org/D148139

In D140790#4257115, @dhoekwater wrote:

/// Return the fallthrough block if the block can implicitly
/// transfer control to the block after it by falling off the end of
/// it. If an explicit branch to the fallthrough block is not allowed,
/// set JumpToFallThrough to be false. Non-null return is a conservative
/// answer.
MachineBasicBlock *getFallThrough(bool JumpToFallThrough = false);

The documentation for this function seems at odds with the behavior.

The documentation suggests that, if JumpToFallThrough is false, then getFallThrough will return a nullptr if the basic block ends with an explicit branch to the fallthrough block.

However, the opposite is true. For a block that ends in an unconditional jump to the fallthrough block, if JumpToFallThrough == true, then the function will return nullptr. If JumpToFallThrough == false, then it will return the address of the fallthrough block.

Is this the intended behavior?

In D147424#4239901, @foad wrote:

This is pretty much the same as D133334 right?

I'm still not too happy about emitting warnings without using proper diagnostics. The reason is that when the disassembler is being used as a library then it's probably not appropriate for it just to write warnings to stderr.

removed debug prints

minor nits

Sounds good, thanks for the review @rampitec

• Adding tests for gfx90a and gfx940