LGTM, Thank.
Seems reasonable.

gentle ping for review.

gentle ping for review.

address comments

In D142309#4102284, @labath wrote:

In D142309#4075745, @JDevlieghere wrote:

LGTM, but I wonder when we'll be able to get rid of RenderScript completely (CC @labath)

I am not going to stand in your way. I usually redirect these questions to @srhines, but I understand he's moved on to other things as well.

remove old code

use function to avoid code duplication

remov blank

clang-format

In D142309#4075745, @JDevlieghere wrote:

LGTM, but I wonder when we'll be able to get rid of RenderScript completely (CC @labath)

gentle ping for review.

In D142826#4098076, @hctim wrote:

Hey, looks like this broke the x86_64-linux sanitizer buildbot: https://lab.llvm.org/buildbot/#/builders/37/builds/19910

You can reproduce the bot using the instructions at https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild (substitute buildbot_fast.sh for buildbot_cmake.sh to get the right one, but I don't think using the official buildscript is necessary. This looks like a 2-stage compile is not currently -Wextra -Werror clean:

/b/sanitizer-x86_64-linux/build/llvm-project/llvm/lib/ProfileData/InstrProfReader.cpp:177:41: error: result of comparison 'uint64_t' (aka 'unsigned long') > 18446744073709551615 is always false [-Werror,-Wtautological-type-limit-compare]
  if (uint64_t(Buffer->getBufferSize()) > std::numeric_limits<uint64_t>::max())
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/b/sanitizer-x86_64-linux/build/llvm-project/llvm/lib/ProfileData/InstrProfReader.cpp:227:41: error: result of comparison 'uint64_t' (aka 'unsigned long') > 18446744073709551615 is always false [-Werror,-Wtautological-type-limit-compare]
  if (uint64_t(Buffer->getBufferSize()) > std::numeric_limits<uint64_t>::max())
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I tried to whack-a-mole those two bugs (see my two commits on https://lab.llvm.org/buildbot/#/builders/37/builds/19926), but there's more latent bugs hanging around.

Probably worth reverting this and the warnings need to be fixed inside clang/llvm first, then we can make it the default.

In D142826#4096348, @aaron.ballman wrote:

LGTM, thank you!

address comment

In D142323#4093787, @0xdc03 wrote:

Okay, I have fixed the code and I think the patch is ready now. It turns out you have to specify the number of commits to push to the patch, arc doesn't pick up on it automatically. Also, in my case it didn't pick up on the fact that I already had a patch (or is it revision?) made, so I had to use the command arc diff HEAD~~ --update D142323 and everything seems fine.

I use arc diff to upload and update the patch every time (I heard people saying arcanist is smart) and arc patch D142323 to review the author's patch.

Thank you so much for reviewing my first patch :>

address comments

In D142323#4092577, @0xdc03 wrote:

@xgupta I have updated the comment in the code listing to match the code that creates the function. I think it is better to keep the ExitOnError variable as I was quite confused as to what ExitOnErr was when reading the tutorial and I only realized it once I scrolled to the end and found the declaration. If this seems correct to you, can you land this patch for me as I do not have commit access? Please use "Dhruv Chawla <dhruv263.dc@gmail.com>" to commit the change.

Thanks!

PS: I am a little concerned if I updated my revision correctly: I used arc diff --update to commit my second change, and I am not sure if that is correct as phabricator is only showing the diff against the second commit...

In D142826#4090415, @aaron.ballman wrote:

Heh, I literally started working on this on Friday afternoon. :-D

Yeah, I saw someone asking about it on discord on Sunday. so thought to look.

update release note

address comment

Thank you for the patch!

update test cases

In D142633#4086574, @efriedma wrote:

It looks like the definition of stores has the same "value of 0 or" language?

Otherwise looks fine.

sorry, I missed and committed, you have also added a new reviewer.

In D142308#4080297, @Meinersbur wrote:

Thank you for the patch. However, we do not patch ISL in the LLVM directly, but instead report upstream to isl-development@googlegroups.com and then update the LLVM copy.

I think this has already been fixed upstream in https://repo.or.cz/isl.git/blobdiff/f7b0ef95845d57fc839090d5075b84fa706eeb15..f035f41be325a5e173d6531f384ded9799943d87:/isl_coalesce.c . I just updated the LLVM copy. The funcion now reads

	if (any) {
		if (isl_tab_rollback(info->tab, snap) < 0)
			return isl_stat_error;
		info->bmap = isl_basic_map_cow(info->bmap);
		info->bmap = isl_basic_map_free_inequality(info->bmap, 2 * n);
		if (!info->bmap)
			return isl_stat_error;

		return fix_constant_divs(info, n, expanded);
	}

Please check whether the problem still occurs. I cannot check without a test case.

Ah, Thanks. Then it is fine. I will drop this revision since fixed is already there.

WIP && TODO- Fixed test

add test cases

In D138757#4078593, @nickdesaulniers wrote:

Thanks for the work on this patch! MIR tests aren't the most fun to write/generate, but doing so builds muscle!

address comments

fix build

In D142309#4075616, @jingham wrote:

I don't think this code is going to compile, is it? You took out the if (!buffer) { but left the RHS } so now the scopes are unbalanced. I think you should just change how the shared pointer is defined - which is presumably the substance of the patch - and leave everything after as is.

address comments

Ah, this particular fault is because I recently added a new element to MIR files. It would appear that you're using an older version of llc to read the output of a newer llc -- you should be able to just rebuild llc and it will work, or in the worst case, rebase onto a commit more recent than rG9f8544713ad8e57.

use --llc-binary build/bin/llc to correcting update test case

update test case for CHECK lines.

In D142304#4074211, @aaron.ballman wrote:

I looked through the code base and I think this fix is good but benign (it doesn't look like it'll impact anything that I can see). So LGTM without tests.

In D142338#4073826, @tbaeder wrote:

LGTM, assuming the pre-commit CI failures are unrelated.

In D142304#4073573, @aaron.ballman wrote:

LGTM but can you find a test case that would show we don't regress this in the future?

address comments

address comments

In D142341#4072810, @Michael137 wrote:

Looks correct to me, given we seem to allow valobj_sp to be null. Though we seem to be doing it in a lot of other places too around this file. So could fix those all at once?