Page MenuHomePhabricator

[X86] Encode global symbol address in small code model
ClosedPublic

Authored by weiwang on Oct 13 2020, 1:52 PM.

Details

Summary

In small code model, program and its symbols are linked in the lower 2 GB of the address space. Try encoding global symbol address even when the range is unknown in such case.

Diff Detail

Event Timeline

weiwang created this revision.Oct 13 2020, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2020, 1:52 PM
weiwang requested review of this revision.Oct 13 2020, 1:52 PM
weiwang edited the summary of this revision. (Show Details)Oct 13 2020, 1:57 PM
weiwang edited reviewers, added: craig.topper; removed: hoyFB.Oct 13 2020, 2:27 PM

Is this something we can detect in X86DAGToDAGISel::isSExtAbsoluteSymbolRef? We already have a pattern for X86sub_flag that calls that. Same for a bunch of other instructions.

Would be good to share benchmark/performance numbers you have for this change too. Thanks.

Thanks for the comment, Craig.

isSExtAbsoluteSymbolRef does seem to check for the width of immediate. If the immediate can be encoded directly, the node should be replaced with one of the SUB64ri* nodes. I think the X86Wrapper node can be replaced with a corresponding imm node if conditions are met, then the matching can proceed.

Would be good to share benchmark/performance numbers you have for this change too. Thanks.

Sure. This change is to address some codegen difference we saw between clang and gcc from internal workloads. Gcc seems to prefer encoding address immediate into cmp in small code model. With the change, we saw 1% perf improvement on average across multiple workloads.

Thanks for the comment, Craig.

isSExtAbsoluteSymbolRef does seem to check for the width of immediate. If the immediate can be encoded directly, the node should be replaced with one of the SUB64ri* nodes. I think the X86Wrapper node can be replaced with a corresponding imm node if conditions are met, then the matching can proceed.

I was thinking that if we don't have range information from getAbsoluteSymbolRange, but the Width passed to isSExtAbsoluteSymbolRef is 32 and the code model is small we could return true?

Thanks for the comment, Craig.

isSExtAbsoluteSymbolRef does seem to check for the width of immediate. If the immediate can be encoded directly, the node should be replaced with one of the SUB64ri* nodes. I think the X86Wrapper node can be replaced with a corresponding imm node if conditions are met, then the matching can proceed.

I was thinking that if we don't have range information from getAbsoluteSymbolRange, but the Width passed to isSExtAbsoluteSymbolRef is 32 and the code model is small we could return true?

Yes, I think it is possible to do that. The similar check is also done in X86DAGToDAGISel::selectMOV64Imm32.

weiwang updated this revision to Diff 300397.EditedOct 23 2020, 1:40 PM
weiwang edited the summary of this revision. (Show Details)

Sorry for the long delay. Got held up by some other task.

Update:

  1. modified change as suggested.
  2. fixed several test cases due to the change.
  3. added a new test case.
This revision is now accepted and ready to land.Oct 24 2020, 10:33 PM
weiwang retitled this revision from [X86] Encode global symbol address in sub if possible to [X86] Encode global symbol address in small code model.Oct 26 2020, 9:50 AM
weiwang edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Oct 26 2020, 11:14 PM
This revision was automatically updated to reflect the committed changes.