Page MenuHomePhabricator

[WIP][TableGen][GlobalISel] Account for HwMode in RegisterBank register sizes
AcceptedPublic

Authored by lewis-revill on Mar 11 2020, 10:15 AM.

Details

Summary

This patch adds logic for determining RegisterBank size to RegisterBankInfo, which allows accounting for the HwMode of the target. Individual RegisterBanks cannot be constructed with HwMode information as construction is generated by TableGen, but a RegisterBankInfo subclass can provide the HwMode as a constructor argument. The HwMode is used to select the appropriate RegisterBank size from an array relating sizes to RegisterBanks.

Targets simply need to provide the HwMode argument to the <target>GenRegisterBankInfo constructor. The RISC-V RegisterBankInfo constructor has been updated accordingly (plus an unused argument removed).

Diff Detail

Event Timeline

lewis-revill created this revision.Mar 11 2020, 10:15 AM
simoncook accepted this revision.Mar 13 2020, 3:26 PM

Patch makes sense, and looking at the newly generated XRegisterBank.inc files, all LGTM

This revision is now accepted and ready to land.Mar 13 2020, 3:26 PM
This revision was automatically updated to reflect the committed changes.
akuegel added inline comments.
llvm/lib/CodeGen/GlobalISel/RegisterBankInfo.cpp
61

bkramer already commented on this on the llvm-commits mailing list. Copying it here for visibility:

This can write to a global variable, which is thread-hostile. I see
tsan complain about concurrent writes to llvm::X86::GPRRegBank.

I reverted the patch (commit baa6f6a7828a46c37b96227282938717220f8b34).

My initial thoughts are that this may only be an issue if there can be potentially more than one instance of RegisterBankInfo? And indeed, if it is possible for each of these to have been constructed with differing HwModes? I'll try and look into it in more detail and find out.

lewis-revill reopened this revision.Mar 23 2020, 9:27 AM
This revision is now accepted and ready to land.Mar 23 2020, 9:27 AM

I'm lacking an understanding of how concurrency is expected to work within LLVM, but since there is only one RegisterBankInfo constructed per subtarget I don't see how there is a problem with updating the global RegBanks array? Likewise the HwMode is constant for the subtarget, so the write will be of the same value.

I'm lacking an understanding of how concurrency is expected to work within LLVM, but since there is only one RegisterBankInfo constructed per subtarget I don't see how there is a problem with updating the global RegBanks array? Likewise the HwMode is constant for the subtarget, so the write will be of the same value.

There can be many subtargets around, and those can be created concurrently. Having state in a global variable is not going to work.

lewis-revill retitled this revision from [TableGen][GlobalISel] Account for HwMode in RegisterBank register sizes to [WIP][TableGen][GlobalISel] Account for HwMode in RegisterBank register sizes.
lewis-revill edited the summary of this revision. (Show Details)

Rewrote the global RegBanks array to be constant. Instead of modifying RegisterBanks after construction, provide HwMode to the constructor of RegisterBankInfo and move size calculation logic for RegisterBanks to that class.