This is an archive of the discontinued LLVM Phabricator instance.

[LLVM][X86][Goldmont] Adding new target-cpu: Goldmont
ClosedPublic

Authored by m_zuckerman on Jun 22 2017, 3:20 AM.

Diff Detail

Event Timeline

m_zuckerman created this revision.Jun 22 2017, 3:20 AM
delena added inline comments.Jun 22 2017, 4:04 AM
lib/Target/X86/X86.td
467

slm is a legacy alias. Maybe you should remove glm?

Don't add the SMAP feature flag. We should only have feature flags that are used by isel.

RKSimon added a subscriber: RKSimon.

Add goldmount to llvm\test\CodeGen\X86\cpus.ll

You might want to add it to atom-sched.ll and atom-call-reg-indirect.ll as well

lib/Target/X86/X86Subtarget.h
518

It'd be better if this is wired into something so you can test it - lea fixup or tti cost models would be the most straightforward.

Although ideally X86ProcFamilyEnum would die and the atom-style cpus would be driven by features/cost models like other cpus.

craig.topper edited edge metadata.Jun 23 2017, 9:53 AM

Can you add goldmont to getHostCPUName in lib/Support/Host.cpp assuming you have the family/model/stepping info for it? Also need to add a RUN line to test/CodeGen/X86/cpu.ll

m_zuckerman marked an inline comment as done.EditedJun 25 2017, 6:44 AM

Don't add the SMAP feature flag. We should only have feature flags that are used by isel.

Adding new feature with predicted to instructions. According to the ISA, there is no define intrinsics for the SMAP feature only two instructions. New review can be found in D34603: [X86][LLVM]Adding SMAP feature and test.

m_zuckerman marked an inline comment as done.

removing SMAP feature.

Were you able to get the family/model/stepping info for lib/Support/Host.cpp?

A search of the linux kernel definitions http://cinnabar.sosdg.org/~qiyong/qxr/linux/source/arch/x86/include/asm/intel-family.h#L62 suggest that steppings 0x5c, 0x7a, and 0x5f are all goldmonts.

lib/Target/X86/X86.td
465

Does GLM have faster PMULLD than silvermont or should we carry over FeatureSlowPMULLD to here?

m_zuckerman marked an inline comment as done.

Should we OR in isGLM() every place we use isSLM() today?

@zvi or @igorb Should all the places that use isAtom() also include isSLM() and isGLM()? Should isAtom() be renamed to isBonnell()? isAtom sounds like it implies any Atom processor, but it really doesn't.

igorb edited edge metadata.Jun 27 2017, 1:34 AM

Should we OR in isGLM() every place we use isSLM() today?

@zvi or @igorb Should all the places that use isAtom() also include isSLM() and isGLM()? Should isAtom() be renamed to isBonnell()? isAtom sounds like it implies any Atom processor, but it really doesn't.

Hello Craig,
I think this patch should focus on functionality only . All performance related issues may be resolved as separate patches following careful study of Intel optimization manual.
https://software.intel.com/sites/default/files/managed/9e/bc/64-ia-32-architectures-optimization-manual.pdf

RKSimon edited edge metadata.Jun 27 2017, 1:40 AM

Should we OR in isGLM() every place we use isSLM() today?

@zvi or @igorb Should all the places that use isAtom() also include isSLM() and isGLM()? Should isAtom() be renamed to isBonnell()? isAtom sounds like it implies any Atom processor, but it really doesn't.

Hello Craig,
I think this patch should focus on functionality only . All performance related issues may be resolved as separate patches following careful study of Intel optimization manual.
https://software.intel.com/sites/default/files/managed/9e/bc/64-ia-32-architectures-optimization-manual.pdf

In which case it'd be better not to add the isGLM() and IntelGLM parts of the patch until they are actually needed. As I've already said, we should ideally be trying to get rid of X86ProcFamilyEnum.

I removed from the list of features two features FeatureSlowDivide64 and FeatureSlowPMULLD. According to the optimization manual ,that Igor added ,this issues were fixed in the GoldMont processor. Regard @RKSimon I agree with you that we want to remove the isglm() until use. But I prefer no touch the IntelGLM since I want to specialize Goldmont from the other ATOM processors .

Regard @RKSimon I agree with you that we want to remove the isglm() until use. But I prefer no touch the IntelGLM since I want to specialize Goldmont from the other ATOM processors .

Thanks that's fine by me.

lib/Target/X86/X86.td
306

Match the indent Atom/SLM

m_zuckerman marked an inline comment as done.Jun 27 2017, 6:35 AM
igorb added a comment.Jun 28 2017, 4:49 AM

LGTM, but maybe wait for another ok before committing.

This revision is now accepted and ready to land.Jun 28 2017, 10:50 AM
This revision was automatically updated to reflect the committed changes.