Page MenuHomePhabricator

[mlir][llvm] Fix llvmBuilder for llvm.alloca so it could emit to non-zero addrspace.
Needs RevisionPublic

Authored by whchung on Apr 28 2020, 9:16 AM.

Details

Summary

For certain targets alloca can take place on a non-zero addrspace.

No tests are added for this patch as existing patches would all pass.

Subsequent patches to mlir-translate would cover cases where alloca on non-zero addrspace takes place.

Diff Detail

Event Timeline

whchung created this revision.Apr 28 2020, 9:16 AM
Herald added a project: Restricted Project. · View Herald Transcript
whchung retitled this revision from Fix llvmBuilder for llvm.alloca so it could emit to non-zero addrspace. to [mlir][llvm] Fix llvmBuilder for llvm.alloca so it could emit to non-zero addrspace..Apr 28 2020, 9:20 AM
whchung edited the summary of this revision. (Show Details)
whchung added a reviewer: nicolasvasilache.
whchung added a project: Restricted Project.
ftynse requested changes to this revision.Apr 28 2020, 10:53 AM

Could you please add a test?

This revision now requires changes to proceed.Apr 28 2020, 10:53 AM
whchung abandoned this revision.Apr 28 2020, 11:55 AM

Closing this patch as D79019 supersedes this one.

ftynse added a comment.EditedApr 29 2020, 8:04 AM

Closing this patch as D79019 supersedes this one.

I would actually advise to keep the patches separate, it's easier to review and track changes in the history.

whchung reclaimed this revision.Apr 29 2020, 11:25 AM
This revision now requires changes to proceed.Apr 29 2020, 11:25 AM

@ftynse I reclaimed the patch. But I'm not sure how to add an extra test specifically for this patch without mlir-translate changes introduced in D79019.

In mlir/test/Target/llvmir.mlir there are tests which checks this particular function how llvm.alloca gets translated to llvm alloca instruction. They are working fine.

And without D79019, there is really no way for mlir-translate to emit alloca on a different addrspace.

@ftynse I reclaimed the patch. But I'm not sure how to add an extra test specifically for this patch without mlir-translate changes introduced in D79019.

In mlir/test/Target/llvmir.mlir there are tests which checks this particular function how llvm.alloca gets translated to llvm alloca instruction. They are working fine.

And without D79019, there is really no way for mlir-translate to emit alloca on a different addrspace.

Good point. That's why I was thinking that the data layout should be available in the std->llvm conversion and llvm.alloca should produce pointers in the right address space to start with, so there is no need to patch it back in the translation.

nicolasvasilache resigned from this revision.Oct 2 2020, 1:25 AM