bjorn3 commented on issue #5490:
I think this looks good. Maybe add a comment at the top of the file referencing the exact file+commit you used? The github url you linked would work.
jameysharp commented on issue #5490:
Thank you, at a quick glance this looks great! A bunch of people are out due to holidays, but if nobody else gets to it first I intend to review this next week. Feel free to comment again if you don't hear anything by the end of next week.
jameysharp commented on issue #5490:
This seems like the right direction!
My only quibble is that I think we should only include 64-bit CPUs (the ones that have
FeatureX86_64
in LLVM). We should be able to generate code for any x86 CPU running in 64-bit mode, and if we can't right now I think we're at least close. But I believe we'd need to do a lot more work to support 32-bit mode, so I think it makes sense to just fail if someone requests a target that can only support 32-bit mode.I haven't yet checked that, for each target we already supported, the set of features is still correct. I believe you probably got it right, but the LLVM tables are hard enough to read that I think it's worth double-checking, just in case.
Overall, I think this is the right patch. If you could remove those 32-bit CPUs I'd appreciate it, and hopefully I'll get to double-checking the features lists soon, or talk somebody else into doing it. :grin:
MozarellaMan commented on issue #5490:
Thank you for the comments! Sorry, I was away a bit. Will have a look at addressing what you've brought up. Thanks for taking the time to double check - I didn't realise the difference regarding the feature implication
MozarellaMan edited a comment on issue #5490:
Thank you for the comments! Sorry, I was away a bit. Will have a look at addressing what you've brought up. Thanks for taking the time to double check - I didn't realise the difference regarding the feature implication. And of course, these are for x86 presets :face_palm:
Last updated: Dec 23 2024 at 12:05 UTC