User Details
- User Since
- Oct 9 2015, 4:06 AM (351 w, 4 d)
Today
Please just merge the two classes. There is no good reason at the moment to separate them.
Abandoning this in favor of removing ManagedStatic entirely via the stack of changes that ends in D129134
Abandoning this in favor of a stack of changes including D129129
LGTM, one suggestion though.
Yesterday
Wed, Jun 29
What happens when the register allocator decides to split a live range of virtual registers here, i.e. if it introduces a COPY?
Tue, Jun 28
Thu, Jun 23
LGTM
Wed, Jun 22
Thanks. This looks pretty good to me, but I'm not familiar with the bitcode reader so perhaps somebody else can still take a look.
Awesome, LGTM
Tue, Jun 21
Thank you. One more thought after sleeping on it: What do you think about !exists? I think !exists<T>("foo") reads nicely as "does there exist a record of type T with name 'foo'?". It's not perfect, but I like that there's less confusion with "instanceof" or "isinstance" operators that some programming languages have.
I'm in favor of doing this, and the change looks mostly straightforward. However, I wonder if, since it is a breaking change in the C ABI, we shouldn't have at least one release cycle where LLVMConstExtractValue (and other functions that are planned to be removed) are marked as deprecated.
LGTM
Most of this looks good to me, I just have a few nits and one bigger question about phi handling inline.
Mon, Jun 20
Can this be abandoned in favor of the !instanceof change?
Hmm, I now saw the earlier discussion and @tra's very good points in it. If !instanceof accepts defs, then it becomes somewhat redundant with !isa, which I hadn't considered. Can you improve the error checking so that if a record is provided as an argument, the message advises that !isa should be used instead?
I think this change makes sense and it looks good to me, except for one thing: if I were to see an !instanceof without context, I would expect that it is also able to take a record/def as operand. That kind of use is likely to come up eventually, and people shouldn't have to artificial convert the record/def to its name first. So I'm in favor of this change, except I'd ask you to change the implementation (and tests) to support both kinds of use.
Wed, Jun 15
Mon, Jun 13
Thank you for doing this. It looks alright to me.
Thu, Jun 9
Jun 4 2022
Ping
Jun 1 2022
Ping -- are there any substantial comments left? Based on previous feedback this is probably good to go with the changes I made last week?
May 27 2022
Add a simple test case
May 26 2022
Address reviews:
- fix comments
- moved to ArrayRefTest.cpp
- add a check that the mutable array ref elements can be assigned to
May 25 2022
May 24 2022
You can use the "Edit Related Revisions" option in the right-hand side menu of Phabricator to link this revision with the others of the series. I can't really speak for the Clang parts, but the LLVM parts looks reasonable to me modulo some detail comments.
It makes sense to unify that handling, but I object to the term "lane VGPRs". In a way, it's the other way around: the registers that are used for SGPR->VGPR spills are de facto "WWM VGPRs", because they're used in a wave-wide manner, which is the opposite of lane-based usage.
I don't think the premise of this patch is correct. A source-level writelane intrinsic is not allowed to overwrite inactive lanes. If it does, then that's UB. If you want wave-wide writelane in source, the way to achieve that would be to use WWM.
May 16 2022
+1 for creating this stuff using TableGen.
Running TableGen multiple times is a bit annoying for LLVM build times. Have you considered packing all outputs into the same .inc file?
May 11 2022
+1 for doing this, thank you!
Thank you for doing this! Not a detailed review but the interface and approach LGTM.
I'm sorry to say that this patch seems confused about semantics. It lacks clear definitions, and in particular for the shuffle attribute in LLVM IR, you almost certainly just want the already existing convergent instead.
May 10 2022
- address review comments
- build on a factored-out buildReadFirstLane (see D125324)
Having gone over the argument with @ruiling separately, I think he has a point. Have you double-checked whether this change is still required after the related LLPC changes have been made?
May 9 2022
softwqm is used by our implementation of subgroup operations: if helper invocations are there, then subgroup operations must interact with them.
May 4 2022
Thank you for the summary.