This is an archive of the discontinued LLVM Phabricator instance.

[Polly][acc] Do not statically dispatch into IslNodeBuilder's createFor
ClosedPublic

Authored by philip.pfaffe on Oct 27 2017, 1:58 AM.

Details

Summary

When GPUNodeBuilder creates loops inside the kernel, it dispatches to
IslNodeBuilder. This however is surprisingly dangerous, since it accesses the
AST Node's user through the wrong type. This patch fixes this problem by
overriding createFor correctly.

This fixes PR35010.

Event Timeline

philip.pfaffe created this revision.Oct 27 2017, 1:58 AM
Meinersbur accepted this revision.Oct 27 2017, 2:23 AM
Meinersbur added a subscriber: Meinersbur.

LGTM

include/polly/CodeGen/IslNodeBuilder.h
354

Is there some change of meaning to the second argument, or why renaming it?

This revision is now accepted and ready to land.Oct 27 2017, 2:23 AM
philip.pfaffe added inline comments.Oct 27 2017, 2:39 AM
include/polly/CodeGen/IslNodeBuilder.h
354

Yes, the meaning changed. Before, KnownParallel was used as a hint to determine whether the created loop should be marked parallel by the Annotator. This analysis now happens in IslNodeBuilder::createFor and is then passed into this method here.

This patch is missing a testcase, because I don't know how to properly test this.

Okay to commit without?

For me this is OK. We cannot reliably test undefined behavior. Tools like TypeSanitizer exist for this purpose.

This revision was automatically updated to reflect the committed changes.