In D64936#1593918, @brzycki wrote:

LGTM. I am assuming this passed test-suite and ninja check runs?

In D64936#1593013, @grandinj wrote:

Just a comment from a bystander:

Isn't there some kind of verifier pass that clang runs (perhaps under some expensive-checks flag) after every pass?

If such a thing exists, checking that BlockAddress nodes are properly cleaned seems like something it should do.

Add move assignment operator that preserves the uselist order and use
std::move instead of std::copy. This preserves the original order of operands instead
of swapping them, which is slightly better IMO as swapping would mean that the order
of the remaining operands is dependent on the order in which we remove incoming values.

Remove unnecessary checks.

I would like to take another look, but do not want to block this for too long. Please feel free to commit if you don't hear from me by Monday.

Thanks for the feedback, setting DYLD_LIBRARY_PATH as suggested does what I want :)

Fix wording in assert messages.

Any chance you could have a quick look Jessica?

Any chance you could have a quick look Jessica?

LGTM, thanks. We iterate over BlocksWithDeadTerminators later on, which can cause edges to BBs to be removed a different order.

Ping. @efriedma do you have any thoughts about the general direction of this patch?

In D59995#1584967, @mstorsjo wrote:

This triggers failed asserts in some cases, reproable with https://martin.st/temp/loadimage-preproc.cpp.xz, with clang++ -target i686-w64-mingw32 -c -O3 loadimage-preproc.cpp. Will post a proper bug report later today, unless someone else beats me to it.

Regarding tests, I think we have unit tests for most functions in LoopInfo, it would be good to add a test there for the new function too, if you decide to add it

IMO this seems like quite a specific function to add (there are other blocks that might be interesting to filter ). I think we could achieve the same thing with a filter_range on the caller side without much extra code and with it being slightly more explicit. But that’s quite subjective, so feel free to disregard.

clang-format, small refactor

For some targets the LICM pass can result in sub-optimal code in some
cases where it would be better not to run the pass, but it isn't
always possible to suppress the transformations heuristically.

I've reverted the patch, because the bitcode file caused an out-of-memory error on a builder.

In D64088#1579463, @Meinersbur wrote:

To not to be stuck in details and not block the dependence graph patches, we maybe should land the patch and work on it in-tree when its users materialize?

In D64128#1576391, @rjmccall wrote:

I wouldn't favor adding something really obscure that was only useful for clang, but I think builtins to set and clear mask bits while promising to preserve object-reference identity would be more generally useful for libraries. For example, there might be somewhere in libc++ that could take advantage of this.

Improve error message.

ping

LGTM thanks. Please wait with committing a day or so, in case Hal or Eli have additional comments.

ping

In D64088#1570715, @bmahjour wrote:

In D64088#1568644, @lebedev.ri wrote:

Tests missing.

The tests will be provided with the patches that extend the DirectedGraph and actually build an instance of it. I realize this patch introduces code without tests, but this is all in an effort to keep the reviews small. If there are any other suggestions on how to break up the reviews I'd be more than happy to look into it.

In D64128#1572504, @rjmccall wrote:

I would be opposed to any addition of a -f flag for this absent any evidence that it's valuable for some actual C code; this patch appears to be purely speculative. I certainly don't think we should be adding it default-on. Your argument about alignment is what I was referring to when I mentioned that this is enforcing alignment restrictions in places we traditionally and intentionally haven't.

Note that it won't help Clang's major uses of PointerIntPair and PointerUnion because our traits say that types like Decl and Type have more alignment bits than their formal alignment would indicate, and PointerIntPair occupies alignment bits starting with the most significant.

In D64359#1576307, @etiotto wrote:

I can use breadth_first from ADT/BreadthFirstIterator.h to collect the loops in breadth-first order. Arguably the getLoopsInPreorder() member functions could also be removed in favor of using ADT/DepthFirstIterator.h (need to confirm whether that does a preorder traversal).

LGTM, thanks. For the commit message, it would be great if you could prefix the area the patch falls in, e.g. [CloneFunction].

AFAIK, GraphTraits is implemented for Loop and you should be able to just use breadth_first from ADT/BreadthFirstIterator.h. It should return a breadth-first iterator range over the sub loops. Given it is a one-liner, I am not sure we would need to add a special method to LoopBase.

Marking as requiring changes while the comments are being addressed.

LGTM, thanks for the alternative approach. It would be great if you could add a sentence about the reasoning (more consistent with the rest of the Loop APIs) in the commit message.

Addressed Ayal's comments, thanks and sorry for the long delay, it somehow dropped off my radar.

LGTM, thanks!

I assume @reames marked it as requiring changes due to my comments, but it might be good to wait till early next week, in case he has additional comments.

In D64128#1569836, @hfinkel wrote:

In D64128#1569817, @rjmccall wrote:

The pointer/integer conversion is "implementation-defined", but it's not totally unconstrained. C notes that "The mapping functions for converting a pointer to an integer or an integer to a pointer are intended to be consistent with the addressing structure of the execution environment.", and we do have to honor that. The standard allows that "the result ... might not point to an entity of the referenced type", but when in fact it's guaranteed to do so (i.e. it's not just a coincidental result of an implementation decision like the exact address of a global variable — no "guessing"), I do think we have an obligation to make it work. And on a practical level, there has to be *some* way of playing clever address tricks in the language in order to implement things like allocators and so forth. So this makes me very antsy.

I don't disagree. But I believe the question is if we have:

int *x = malloc(4);
int *y = malloc(4);
if (x & ~15 == y) {
  *(x & ~15) = 5; // Is this allowed, and if so, must the compiler assume that it might set the value of *y?
}

Thanks for the quick responses and the helpful comments. Thank you very much Hal, for summarizing the argument from previous discussions. My initial understanding indeed was that by generating ptrmask directly for C/C++ expressions, we can circumvent the issues that come with ptrtoint/inttoptr in LLVM.

Maybe we could make the semantics of callbr a bit more explicit in LangRef?

LGTM, thanks!

Handle nonnullness requirement for the use of getArgumentAliasingToReturnedPointer
in isKnownNonZero, as suggested.

The AMDGPU fix was committed, so this should be ready to go in :) @reames, it would be great if you could accept it on Phab

Use IgnoreParens().