Page MenuHomePhabricator

[ORC] Add host CPU name and sub-target features to JITTargetMachineBuilder::detectHost()
ClosedPublic

Authored by dcaballe on Aug 5 2019, 10:59 AM.

Details

Summary

We use JITTargetMachineBuilder::detectHost() in MLIR to create a target machine for the host CPU and noticed that the returned JITTargetMachineBuilder is not properly initialized with the host CPU name and sub-target features. This patch adds that information to the JITTargetMachineBuilder created in JITTargetMachineBuilder::detectHost().

Please, let me know if there is a simple way to add a test for this.

Thanks!
Diego

Diff Detail

Repository
rL LLVM

Event Timeline

dcaballe created this revision.Aug 5 2019, 10:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2019, 10:59 AM
rogfer01 removed a subscriber: rogfer01.Aug 5 2019, 11:14 AM
lhames accepted this revision.Aug 5 2019, 12:57 PM

Looks good to me. Thanks Diego!

I think it's ok to commit without a test for now. If detectHost() starts getting more complicated we could add a unit test to verify its behavior (basically: the resulting JTMB's fields match the values from getHostCPUName/getHostCPUFeatures/whatever-else-we-add). I don't think we need that yet though.

This revision is now accepted and ready to land.Aug 5 2019, 12:57 PM

Thanks for the quick response, Lang! I'll proceed with the commit in a while, in case somebody else has any comments.

This revision was automatically updated to reflect the committed changes.
plotfi added a comment.Aug 5 2019, 4:34 PM

I've reproed this locally. Reverting.

plotfi added a comment.Aug 5 2019, 4:47 PM

Reverted in r367954. Please try landing again @dcaballe

Please make sure to verify it builds on Linux.

Thanks, @plotfi. Sorry about that. It passed with gcc in my local machine. I'll check with Clang.

plotfi added a comment.Aug 5 2019, 5:25 PM

Thanks, @plotfi. Sorry about that. It passed with gcc in my local machine. I'll check with Clang.

Ah yeah no worries. I've had it happen where the released clang worked fine locally but the bot Top-Of-Tree clang exposed a bug in my code before :-P.