- User Since
- Oct 11 2019, 12:14 PM (79 w, 22 h)
Tue, Mar 30
Thu, Mar 25
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.
Mon, Mar 22
Fri, Mar 19
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.
@JDevlieghere I personally have no skin in this game. This is actually quite an obnoxious development for me since my linux machine is stuck on an old version and Python 3.5 is the newest version in the repos. I sent a message to llvm-dev so hopefully this will get hashed out there. You are right though, that the linked page does not actually say "minimum requirement". I just assumed that since code was going in unchallenged that requires it, that it must be true.
Jan 28 2021
Nice. I think the separate cost and budget make this one easier to read. Address @spatel's issues then this looks good to me.
The differential revision is D93097 if anybody is curious :P
Jan 27 2021
Jan 26 2021
Aside from the change to the assert, this version looks good to me.
Jan 25 2021
Aside from the question of InstructionBudget this looks good to me.
I'd like you to reconsider adding InstructionBudget. I think this new class is just trying to paper over people's misunderstanding of how InstructionCost works. This wouldn't be a huge issue, except now we're going to have to add a million redundant operator definitions to it ("why can't I multiply budgets?") so it's going to become a maintenance burden.
Aside from the one nit, this looks good to me.
Jan 21 2021
Jan 14 2021
Jan 12 2021
Jan 11 2021
Question: Would there be an expectation that unsupported build systems are in a good state for LLVM version branches? If I checkout the git tag for a final LLVM release, should I be able to expect that the build system is functional at that commit?
Jan 6 2021
Jan 5 2021
Have a couple nits, but seems reasonable to me. Somebody else should review as well.