Page MenuHomePhabricator

invariant.group and invariant.group.barrier docs
ClosedPublic

Authored by Prazek on Jul 21 2015, 6:30 PM.

Diff Detail

Event Timeline

Prazek updated this revision to Diff 30303.Jul 21 2015, 6:30 PM
Prazek retitled this revision from to invariant.group stuff.
Prazek updated this object.
nlewycky added inline comments.
docs/LangRef.rst
6872

Typo "referenece"

6876

Typo "loados"

6877

Consider:
(but see the @llvm.invariant.barrier() intrinsic which affects when two pointers are considered the same).

6980

Typo "referenece"

6984

Typo "loados"

6985

Consider:
(but see the @llvm.invariant.barrier() intrinsic which affects when two pointers are considered the same).

11567–11568

I'd remove "specifies that given a pointer," and switch "the first" with "its argument" and "produces" with "returns":

The '``llvm.invariant.group.barrier``' intrinsic returns another pointer that aliases its argument but which is considered different for the purposes of load/store !invariant.group metadata.
11580–11581

Consider:
This intrinsic produces a copy of its argument, except that it is a distinct pointer for the purposes of the !invariant.group metadata.

rsmith added a subscriber: rsmith.Jul 21 2015, 6:41 PM
rsmith added inline comments.
docs/LangRef.rst
6874

existance -> existence

6876

have the same -> has the same
loados -> loads

6877

This suggests that any such call could affect the loads. I think the right model is that the @llvm.invariant.group.barrier returns a different pointer value. That makes this parenthetical redundant, since "same pointer value" is already a side-condition.

6932–6934

invariant.group.barrier -> invariant.group

6980–6985

Same comments as above.

6982

existance -> existence

6984

that have -> that has
loados -> loads

11575

might changed -> might have changed

... but I don't think that's quite what we want to say. Something like:

the pointer to the memory for which the invariant no longer holds

maybe?

11580–11581

I don't think it's accurate to say this affects the "next" load/store. The text in the "Overview" section seems like a good fit for a semantic description of the intrinsic. Maybe in the overview you could put something more general like "The <...> intrinsic can be used when an invariant established by !invariant.group metadata no longer holds, to obtain a new pointer value that does not carry the invariant information."

11583–11587

Delete this, we already have this heading on line 10151.

Prazek updated this revision to Diff 30317.Jul 21 2015, 8:57 PM
Prazek marked 16 inline comments as done.
reames added a subscriber: reames.Jul 31 2015, 2:54 PM
reames added inline comments.
docs/LangRef.rst
6875

The fact that it's the llvm::Value of a pointer that matters and not it's runtime value (aka the actual address) is non-obvious from this description.

6876

The comma in "value, that" seems misplaced. I think you intend something like the following:
"every load and store to the same pointer operand within the same invariant group (i.e. index), can be assume to load or store the same bit value..."

6879

Stray change, please submit separately.

6980

Given the repetition here, I'd suggest adding a new subsection defining the notion of an invariant group and then reference that from the per instruction description.

Prazek updated this revision to Diff 33878.Sep 2 2015, 4:24 PM
Prazek retitled this revision from invariant.group stuff to invariant.group and invariant.group.barrier docs.
Prazek added reviewers: reames, rsmith, majnemer, echristo.
Prazek marked 3 inline comments as done.
Prazek added a subscriber: llvm-commits.

Getting ready to push it to master with invariant.group.barrier implementation

nlewycky added inline comments.Sep 8 2015, 1:50 PM
docs/LangRef.rst
4427

"atached" --> "attached"

4430

"can be assume" --> "can be assumed"

4430

What does it mean to be assumed to store the same bit value? In the case of loads I think it means that the load must return the same value. Is the store stating that the store must be storing the same value, that is the value operand of the two stores is bit equivalent?

4582–4583

This should either be removed or committed independently.

11545

"it's" --> "its"

11554

Either revert this newline or commit it independently if correct.

Prazek marked 5 inline comments as done.Sep 14 2015, 4:12 PM
Prazek added inline comments.Sep 14 2015, 4:17 PM
docs/LangRef.rst
4430

I don't understand what you are saying. It means that 2 stores with the same invariant.group and same operand must store the same value

rsmith added inline comments.Sep 14 2015, 4:28 PM
docs/LangRef.rst
4430

Remove the word "bit" here, just "load or store the same value".

4444–4445

You should insert a call between these two, since otherwise we can assume they load the same value because the value cannot have changed between the two loads.

4454

Again, insert a call before this load, since otherwise we can prove that this loads 42 because we just saw a store of 42 to the same region of memory.

Prazek updated this revision to Diff 34760.Sep 14 2015, 5:09 PM
Prazek marked 5 inline comments as done.
rsmith accepted this revision.Sep 14 2015, 5:23 PM
rsmith edited edge metadata.

LGTM, I think you've addressed everyone's feedback.

docs/LangRef.rst
4427

instruction -> instructions

4430

Remove the comma here.

This revision is now accepted and ready to land.Sep 14 2015, 5:23 PM
Prazek updated this revision to Diff 34763.Sep 14 2015, 5:41 PM
Prazek edited edge metadata.
Prazek closed this revision.Sep 15 2015, 11:34 AM