jdoerfert requested changes to this revision.
jdoerfert added a comment.
This revision now requires changes to proceed.

First, apologies for being late, I didn't properly monitor the list recently.

---

This diff is impossible to review and later to understand.

I would ask you to split it, at least for review purposes, such that (1) lang 
ref changes, (2) code changes, (3) test changes are separate.
Please also upload it with a full diff (at least for (1) and (2)).

The revision title needs to be updated (maybe add `[LangRef]` or `[Attr]`), and 
the commit message needs more explanation of the reasons and semantics.

---

Lang Ref (https://reviews.llvm.org/differential/changeset/?ref=2023449 <- so 
people can find it)

I think the spelling is not in line with other attributes and needs to be 
changed.

I'm unsure if we want the name `frozen` as it is less helpful to anyone now 
familiar with the frozen instruction.
In a different thread we concluded that we need some sort of `nopoison` as an 
attribute to convey the behavior is UB if the value would be poison.
I would very much prefer a self explanatory spelling here, especially since 
`nopoison` will be derived from sources other than the frozen instruction.

In the lang ref this is listed with and shown as string attribute. It should be 
with the regular ones (as it is implemented as such). It is also not target 
specific.

---

All in all we need to open this up to more people and go over the lang ref 
changes again.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to