This is an archive of the discontinued LLVM Phabricator instance.

SimplifyCFG: prevent certain transforms on convergent operations
AcceptedPublic

Authored by nhaehnle on Aug 9 2020, 7:24 AM.

Details

Summary

These changes are justified by the new controlled convergent
operations semantics. This change is taken from the otherwise obsolete
https://reviews.llvm.org/D68994

Change-Id: I4f8b780e8c69f98389e4094477fd03e88fc95573

Diff Detail

Event Timeline

nhaehnle created this revision.Aug 9 2020, 7:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2020, 7:24 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
nhaehnle requested review of this revision.Aug 9 2020, 7:24 AM
arsenm added inline comments.Aug 11 2020, 7:46 AM
llvm/lib/Analysis/ValueTracking.cpp
4468–4473

Sink this below the convergent check?

nhaehnle marked an inline comment as done.Aug 14 2020, 11:14 AM
nhaehnle added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4468–4473

Will do.

efriedma added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4479

I can't imagine a function that makes sense to mark both convergent and speculatable.

nhaehnle updated this revision to Diff 285708.Aug 14 2020, 11:36 AM
nhaehnle marked an inline comment as done.
  • sink use getCalledFunction() below convergence check in ValueTracking
nhaehnle added inline comments.Sep 7 2020, 7:59 AM
llvm/lib/Analysis/ValueTracking.cpp
4479

An example would be a shuffle operation:

if (cond1) {
  y = shuffle(x, srcIdx, token);
  foo(y);
} else if (cond2) {
  y = shuffle(x, srcIdx, token);
  bar(y)
}

If any(cond1 || cond2) is sufficiently likely, the following is more efficient:

y = shuffle(x, srcIdx, token);
if (cond1) {
  foo(y);
} else if (cond2) {
  bar(y)
}

This transform is correct for the underlying hardware implementation if srcIdx can be out-of-range without causing undefined behavior.

convergent speculatable would still indicate that the transform is forbidden, because it changes the set of communicating threads, but I think we're going to want attributes that partially relax convergent again. In this case, we'd allow changing the set of communicating threads to a superset.

efriedma accepted this revision.Sep 7 2020, 5:19 PM

LGTM

This revision is now accepted and ready to land.Sep 7 2020, 5:19 PM