This is an archive of the discontinued LLVM Phabricator instance.

Add HSAIL target
AbandonedPublic

Authored by arsenm on May 13 2015, 8:37 AM.

Details

Reviewers
tstellarAMD
Summary

Just includes patch to add bulk of target, not tests or other prerequisite patches.

Diff Detail

Event Timeline

arsenm updated this revision to Diff 25697.May 13 2015, 8:37 AM
arsenm retitled this revision from to Add HSAIL target.
arsenm updated this object.
arsenm edited the test plan for this revision. (Show Details)
arsenm added a subscriber: Unknown Object (MLST).
arsenm updated this revision to Diff 28070.Jun 19 2015, 6:49 PM

Update for trunk changes, plus a few bug fixes. Add new WIP set of intrinsics.

rafael added inline comments.
lib/Target/HSAIL/CMakeLists.txt
20

Please leave this out of the first version.

Having two code paths in CodeGen in pretty bad, specially when one of them depends on an external library.

arsenm updated this revision to Diff 29132.Jul 6 2015, 3:23 PM
arsenm edited edge metadata.

Remove libHSAIL option / path

rafael added inline comments.Jul 10 2015, 1:05 PM
include/llvm/Support/ELF.h
237
lib/Target/HSAIL/CMakeLists.txt
50

No commented out code please.

lib/Target/HSAIL/HSAIL.h
69

Empty comments?

103

Don't repeat names in comments.

lib/Target/HSAIL/HSAILAlwaysInlinePass.cpp
26

please git-clang-format the patch.

58

range loop?

lib/Target/HSAIL/HSAILAsmPrinter.h
33

Start functions with lowercase letter when you can (or is this missing an override?)

lib/Target/HSAIL/HSAILSection.cpp
15

Just don't create sections instead of having empty section switching.

MatzeB added a subscriber: MatzeB.Jul 10 2015, 1:28 PM

Just some codingstyle issues I found while skimming over a few files

lib/Target/HSAIL/AMDOpenCLKernenv.h
10–12

Needs three slashes. There are similar problems in many of the other files.

16–17

Should be changed to LIB_TARGET_HSAIL_AMDOPENCLKERNELENV_H. There are similar issues in other headers.

lib/Target/HSAIL/HSAIL.h
25–43

Better use static const unsigned or enum values so debuggers can pick those constants up as well.

217–221

Could use doxygen comments here:
/// Target architectures ...
enum ... {

GENERIC, ///< No target specific flavor
...
lib/Target/HSAIL/HSAILAlwaysInlinePass.cpp
24

Would be nice to name the file like the class declared inside.

test/CodeGen/HSAIL/128bit-kernel-args.ll
1

Many of the tests define new prefixes instead of just using "CHECK" should be unnecessary if you just test for 1 variant anyway.

arsenm updated this revision to Diff 29605.Jul 13 2015, 2:18 PM

Update based on review comments.

arsenm added inline comments.Jul 13 2015, 2:22 PM
include/llvm/Support/ELF.h
237

This is being requested

lib/Target/HSAIL/HSAILAlwaysInlinePass.cpp
60

I fixed this in the AMDGPU copy of this pass, so once http://reviews.llvm.org/D11155 is in I'll just remove this file.

lib/Target/HSAIL/HSAILSection.cpp
17

These sections are created by the generic code in the AsmPrinter. This is exactly what NVPTX does currently to fix this.

test/CodeGen/HSAIL/128bit-kernel-args.ll
3

People have expressed regret as having a default of CHECK for this before. The idea is someday it might be useful to add an HSAIL64 or something like that someday.

qcolombet resigned from this revision.Aug 5 2015, 1:19 PM
qcolombet removed a reviewer: qcolombet.
lib/Target/HSAIL/HSAILISelDAGToDAG.cpp
607–609

Does this function really need to be recursive? it seems more complicated than it needs to be.

lib/Target/HSAIL/MCTargetDesc/RawVectorOstream.h
24–26

Is there a reason this isn't in common code?

arsenm added inline comments.Aug 21 2015, 12:10 PM
lib/Target/HSAIL/HSAILISelDAGToDAG.cpp
607–609

This could probably be simplified a lot. The cases for ADD/OR looks unnecessary. It seems to mostly be to workaround the ADD -> OR combine. I don't think LDA needs to be handled here at all. I fixed it so that global addresses are handled during selection, so this shouldn't be seeing the custom node (which doesn't seem to be used anymore)

lib/Target/HSAIL/MCTargetDesc/RawVectorOstream.h
24–26

I think I can completely remove this for now since it should only be used by the BRIGAsmPrinter

arsenm abandoned this revision.Jan 31 2017, 4:58 PM