- User Since
- Oct 11 2019, 12:14 PM (180 w, 1 d)
Feb 3 2022
Nov 2 2021
Oct 14 2021
OK, thanks for the update!
Oct 13 2021
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.
Oct 5 2021
Oct 4 2021
@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.
Oct 1 2021
Sep 30 2021
The old code works for me with visual studio 2019, but fails with clang 7 and clang 12.1.
Sep 29 2021
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
Sep 27 2021
Aug 23 2021
Aug 18 2021
Aug 17 2021
Apr 23 2021
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.
Apr 19 2021
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.
Mar 30 2021
Mar 25 2021
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.
Mar 22 2021
Mar 19 2021
Mar 16 2021
Mar 15 2021
Mar 11 2021
set a var for the minimum bound
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)
Mar 8 2021
@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.
Mar 5 2021
Mar 4 2021
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?
Mar 3 2021
Mar 2 2021
Assuming everybody else is satisfied, LGTM
Feb 26 2021
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.
Feb 25 2021
Feb 24 2021
Feb 19 2021
LGTM. I'll land it for you.
Feb 5 2021
the reland has landed
Feb 4 2021
Fix test, add comment
address code review issues
Feb 3 2021
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
Feb 2 2021
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.
Feb 1 2021
address code review issues
Aside from the nit, this looks good to me.
Jan 29 2021
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.