Page MenuHomePhabricator

Make fixed-abi default for AMD HSA OS
ClosedPublic

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

Details

Summary

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
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
386–388

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
llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-fixed-function-abi-vgpr-args.ll
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
llvm/test/CodeGen/AMDGPU/callee-special-input-sgprs.ll
16

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