Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Make fixed-abi default for AMD HSA OS

Authored by madhur13490 on Feb 9 2021, 6:50 AM.



fixed-abi uses pre-defined and predictable
SGPR/VGPRs for passing arguments. This patch makes
this scheme default when HSA OS is specified in triple.

Diff Detail

Event Timeline

madhur13490 created this revision.Feb 9 2021, 6:50 AM
madhur13490 requested review of this revision.Feb 9 2021, 6:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2021, 6:50 AM
arsenm added inline comments.Feb 9 2021, 8:51 AM

This isn't just changing the default, this is breaking the flag. You should check if the option wasn't specified

rebase, address Matt's comments

arsenm added inline comments.Feb 10 2021, 6:29 AM
2 ↗(On Diff #322623)

You should keep the two checks with the flag on and off

Keep both flags

madhur13490 marked 2 inline comments as done.Feb 10 2021, 8:16 AM
arsenm added inline comments.Feb 11 2021, 3:25 PM

This test should force use the variable ABI. We already have a dedicated test for the fixed ABI for these inputs (callee-special-input-sgprs-fixed-abi.ll)

force varabi on callee-special-input-sgprs.ll

arsenm accepted this revision.Feb 17 2021, 11:29 AM
This revision is now accepted and ready to land.Feb 17 2021, 11:29 AM
This revision was landed with ongoing or failed builds.Feb 19 2021, 7:05 AM
This revision was automatically updated to reflect the committed changes.

Setting this 'true' on everything is expected to be a regression. Specifically any code that uses functions, but doesn't use indirect functions, is expected to be slower than it was before. Suggest reverting this patch.

Alternative would look something like:

  • if there is any indirect call, set this true
  • otherwise, leave it unchanged

That will fix things using indirect calls without regressing everything else.

note: llvm::Function::hasAddressTaken() looks useful for that