• Addressed review comments.

LGTM.

• renamed the nodes and the search function.
• added support for other setcc opcodes, other than EQ and NE.

• Addressed review comments.

In D64281#1593246, @jhenderson wrote:

Don't forget to update the review summary, following your changes to this patch. It still refers to "UpdatableList" which is obviously out of date. Also don't forget to use the "Update related revisions" option to show the dependency this change has on other reviews.

Don't forget this bit.

Thanks for the update. Will be worth adding some reviewers from Apple to see if this change should be IsAAPCS only. I've no more further comments myself besides a small nit on style.

• Moved the codegen test to a separate file
• Added a langref description for this new metadata node.

Some cases can be undone by rematerialization, but not all, and it can involve a lot of effort which increases compile time. The metadata is a pragmatic approach which helps in some cases.

Set android-abi default to 128. Added tests for android and not-android.

Changed the metadata name to llvm.licm.disable

• Rewrite code as suggested in the review

• Set androideabi alignment to 128 bits

• Enable asserts since we will now fix the code while introducing the

LGTM! Thanks!

In D64800#1593760, @sbc100 wrote:

Is there a danger that by moving in this direction we loose some test coverage since we are no longer verifying the libObject can read a know set of binaries, but only that libObject can read the output yaml2obj? i.e. we could evolve the object format in llvm such that these tests would continue pass but the objects would no longer be standards compliant?

I think the pre-canned binaries don't really help us any more than yaml2obj inputs - it would be just as straightforward to tune our object reading code to work with a very specific set of binaries (unintentionally or otherwise) as it would be to work with our yaml2obj output. It's worth noting that the yaml2obj writing code and llvm-readobj reading code are not the same set of code, so we are already testing two different implementations against each other. If we change one and not the other, a test should break, alerting us to a behaviour change. We would need a good reason to change both.

LGTM

ASTImporter.cpp and ASTStructuralEquivalence.cpp looks good to me!

LGTM

In D62462#1595071, @mmpozulp wrote:

@jhenderson, can you think of an input that would cause Symbolizer->symbolizeCode(*Obj, Address) to return an Error?

I went a few calls deep underneath Symbolizer->symbolizeCode(*Obj, Address) and it looks like LLVMSymbolizer and DWARFContext return a default-constructed DILineInfo when they fail to parse the debug info. Can we cause a failure that returns an Error instead? If so we can check whether the Error is actually an ErrorInfo subclass which is how custom errors are created according to error handling Detailed Description. A better implementation of SourcePrinter::printSourceLine() might call handleErrors() instead of consumeError(). Grepping the code for handleErrors and handleAllErrors provides some example uses. First I need some inputs to cause errors so I can see whether any ErrorInfo subclasses are returned and if they have useful context which we can incorporate into our warning. This is my first time wading into LLVM's error handling so please let me know if you have any reading suggestions. In addition to the doxygen docs for Error.h I also found a section on error handling in the programmer's manual.

Okay, the possibility of an AssertZext is an interesting point. So let me try the other way around: What would the MIR at this stage look like to enforce an and?

I think that this may not apply for Android as AFAIK their ABI still requires 128-bit alignment in some cases. Adding some more reviewers from Android.

the variable and option name changed.

Test is updated to show what this patch changes.

In D64972#1593826, @reames wrote:

I'm not quite sure this is the right macro approach.

The comment near the code your modifying makes me thing the whole reasoning behind disabling future peeling and unrolling after the first peel may be flawed. It doesn't make sense to "use up" profiling information. Assuming we correctly updated our profiling when doing the transform, the resulting profile for the loop should indicate that it's cold and thus not profitable to further peel/unroll. If that's not happening, maybe there's another issue in play? (I wonder if your other profile bug fix may help here?)

p.s. I'm opened to being convinced that this is a practical answer, even if not an ideal one. Just make the argument. :)

In D64946#1595185, @nhaehnle wrote:

In D64946#1593675, @rampitec wrote:

In D64946#1593334, @nhaehnle wrote:

How about the following simpler logic:

• if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
• otherwise, insert the COPY after the basic-block prologue

In this case COPY in the prologue also shall be marked as prologue instruction somehow.

You're right, my idea doesn't work.

Please make sure to update the documentation at llvm/docs/CommandGuide/llvm-objcopy.rst to move this switch out of the ELF-specific directory.

Okay thanks, I see the logic now.

A gentle ping. It's a one liner and should be reviewed easily :)

Ping.

In D64946#1593675, @rampitec wrote:

In D64946#1593334, @nhaehnle wrote:

How about the following simpler logic:

• if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
• otherwise, insert the COPY after the basic-block prologue

In this case COPY in the prologue also shall be marked as prologue instruction somehow.

My understanding is that this is mostly related to CWSR. The trap handler has to be able to "replay" the GWS instruction.

Gentle pin..

The AMDGPU changes seem fine to me overall.

Converting a dbg.value into a dbg.declare implicitly extends the duration ("lifetime" in the docs [0]) of the variable location from "until the next dbg.value" to "the entire scope". Consider what happens if we make the modifications here [1] to your test case. Imagine an IR producer that wants to temporarily bind some variable to a field of 'result' (the dbg.value added with DW_OP_deref), and later assigns '1' to the variable with the second dbg.value, to cover the rest of the function.

Thanks! Could you please also add a test to Analysis/ValueTracking/memory-dereferenceable.ll?

added check for aggregate dbg.value.

LGTM

r366668

I am going to commit this to free road for another patches. If during futher discussion(s)
will be decided to keep these or other binaries checked in for something, we can do it
in a follow-up independent commit(s) with appropriate test case(s) and comment(s) about
how exactly binaries were produced and for what we keep them.

looks okay to me.

LGTM

Looks like a good optimisation to me.

• Addressed review comments.

looks reasonable

New patch:
https://reviews.llvm.org/D65064