OK, thanks for the update!

Any news on this? As @mehdi_amini mentions, we have a fork. I would think that the ideal solution is to just merge that PR into our fork, then update the instructions to say to use our repo for arcanist. If our fork of phabricator is not long for this world, I don't think this would make getting rid of it in the future significantly harder.

@aeubanks Sorry, I accidentally pushed this commit. Personally, I think it's fine and obvious (meeting the standards of merging without review), but if you still object let me know and I can revert it.

In D110976#3037605, @smeenai wrote:

Would we want to put up the commit in a fork that we control, so that we're not relying on someone else's fork (which may go away in the future)?

The old code works for me with visual studio 2019, but fails with clang 7 and clang 12.1.

address code review issues:

• fix include guard
• rename mag to Retval
• also, make all the variables PascalCase to silence clang-tidy

address code review issues; clean up the interface

add include guard

I decided to create a new file in llvm/Support rather than take one of the suggestions in Chris's diff thread because:

This patch actually broke my downstream as well. I uploaded a patch to move this to a new file in Support: https://reviews.llvm.org/D110747

In D110594#3026096, @aeubanks wrote:

What's the motivation behind this?
In general I don't think we support this sort of -mllvm behavior.

In D108472#2957668, @nikic wrote:

I'm not sure doing this is a good idea. The nice thing about the current way of testing is that it works both for the new pass manager and the legacy pass manager. That is, people can flip ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER to false, run the test suite, and get good test coverage for the legacy pass manager. Switching to NewPM-only syntax removes that ability.

I prefer an assert, but so long as the compiler produces correct results in the assertion failure case, I guess it's fine for now. I'll not block your patch over it.

I'm not sure I follow why the logic would be far too complicated for an assert? There are asserts that verify the whole dominator tree or the whole function, which seems much more complicated :) Also, IIUC the assert caught a case that was missing from the comment/explanation in the initial patch, so it seems like it was doing what it was supposed to be?

Assuming @c-rhodes is satisfied, this change looks good to me.

I think it's good to write this down. A revert could be interpreted as a hostile action, so the fact that they happen all the time around here should be written down.

lgtm

In D95635#2619804, @serge-sans-paille wrote:

would it make sense to syndicate the minimal version in a single variable, say LLVM_MINIMAL_PYTHON_VERSION, and use it in several place instead of hard-coding the value across multiple files?

set a var for the minimum bound

Ping

Only require if adding test suite targets

I plan to revive this patch. I will try to rig it to only require 3.6 or above is testing is enabled using LLVM_INCLUDE_TESTS. Hopefully if any build bots haven't been fixed by now, they will not be setting this variable to ON (since they clearly aren't running tests)

@ctetreau: We've followed the same route as for llvm.vscale() -> ISD::VSCALE so yes the code generation side is more complete/convenient. Considering we're expecting llvm.experimental.stepvector() to be redundant once there's time to push for the preferred ConstantVector solution, we feel it's better to keep the intrinsic as simple as possible.

lgtm

Aside from the function signature for InstructionCost::map, this patch looks good to me.

If the ISD node is going to get an argument for the step, why not let the new intrinsic have this same argument?

Assuming everybody else is satisfied, LGTM

Does changing the -1's to invalid's constitute a functional change? Are they always checked for? If they are just blindly added to other costs, then they would result in valid costs becoming invalid costs.

LGTM. I'll land it for you.

the reland has landed

Fix test, add comment

address code review issues

I just checked that this passes the expensive checks on my machine, so I think it should be good to go now.

This version LGTM

Fix operator<

A reasonable way ahead would be to just take the change to operator==, but not the change to operator<. We could explicitly codify "all invalid costs are considered to be equal", and this would basically be equivalent to Optional<int>.

Unfortunately, this breaks the build. Apparently several crashes and test failures result from this with the expensive checks. Unfortunately, things are very busy at work, so I don't have time to look into this for the time being.

address code review issues

Aside from the nit, this looks good to me.

This all seams fairly reasonable to me, just some minor nitpicks. I'm not confident in giving this the final approval, but hopefully this can serve as a ping to those who are.