Page MenuHomePhabricator

[x86][SLH] Rm liveness check from data invariance check
ClosedPublic

Authored by zbrid on Nov 14 2019, 4:43 PM.

Details

Summary

SLH had two functions named isDataInvariant and isDataInvariantLoad that
checked whether the passed instruction was data invariant. For some instructions,
if the EFLAGS were dead then they were considered data invariant, otherwise
they were not considered data invariant.

In this patch, I extracted that EFLAGS liveness check and made it
explicit at every call to isDataInvariant and isDataInvariantLoad.
This makes the isDataInvariant function behave more generally
and preserves the liveness check behavior that SLH would like to have.

Tested via llvm-lit llvm/test/CodeGen/X86/speculative-load-hardening*

This is the first step in making these two data invariance checks
available for non-SLH passes. The second step is to move the passes from
SLH to X86InstrInfo.cpp. I'll follow up with a patch that does that.

Diff Detail

Event Timeline

zbrid created this revision.Nov 14 2019, 4:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2019, 4:43 PM
zbrid edited the summary of this revision. (Show Details)Nov 14 2019, 4:48 PM
zbrid updated this revision to Diff 229426.Nov 14 2019, 4:49 PM

[x86][SLH] Update commit message for clarity

rnk accepted this revision.Dec 16 2019, 3:12 PM

lgtm

I was waiting for input from Chandler, but in the absence of that (one month later >_>), I see that this is NFC, and you are really working to make isDataInvariant(Load) shared with other passes. This seems like a step in that direction, and it all makes sense. I think you can go ahead and land this.

This revision is now accepted and ready to land.Dec 16 2019, 3:12 PM
zbrid updated this revision to Diff 248258.Mar 4 2020, 11:09 AM

Rebased and tested. No change to the code.

zbrid added a comment.Mar 4 2020, 11:10 AM

Uh dang. I messed this up. I'm going to close this and make a new change that only removes the liveness check.

zbrid closed this revision.Mar 4 2020, 11:11 AM
zbrid added a comment.Mar 4 2020, 11:17 AM

Oh wait, no this looks fine. Woo hoo.

zbrid reopened this revision.Mar 4 2020, 11:18 AM
This revision is now accepted and ready to land.Mar 4 2020, 11:18 AM
zbrid updated this revision to Diff 248272.Mar 4 2020, 11:55 AM

No changes.

MaskRay accepted this revision.Mar 4 2020, 12:17 PM
MaskRay added a subscriber: MaskRay.
MaskRay added inline comments.
llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp
2222

Delete excess parentheses.

zbrid marked an inline comment as done.Mar 4 2020, 12:41 PM
zbrid added inline comments.
llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp
2222

Hm, I'm not sure which parentheses you mean. On my scan, I don't see any extras.

This revision was automatically updated to reflect the committed changes.
zbrid added a comment.Mar 4 2020, 3:01 PM

Welp, I actually did mess this up. See this for the fix: https://reviews.llvm.org/D75650 Fortunately, it's a NFC to fix the mess up, that is, the code works correctly either way.