This is an archive of the discontinued LLVM Phabricator instance.

[X86] Check immediate before get it.
ClosedPublic

Authored by LuoYuanke on Jun 10 2021, 7:58 AM.

Details

Summary

For CMP imm instruction, when the operand 1 is symbol address we should
check if it is immediate first. Here is the example code.
CMP64mi32 $noreg, 8, killed renamable $rcx, @d, $noreg, @a, implicit-def $eflags
However I am not able to create a small test case to expose this issue.

Diff Detail

Event Timeline

LuoYuanke created this revision.Jun 10 2021, 7:58 AM
LuoYuanke requested review of this revision.Jun 10 2021, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2021, 7:58 AM
lebedev.ri edited the summary of this revision. (Show Details)Jun 10 2021, 8:00 AM
lebedev.ri added a subscriber: lebedev.ri.

llvm-reduce should get you 99% of the way there.

Thank @lebedev.ri. This is triggered by our internal code, the same test case passes with llvm trunk code. BTW, do you think we need to check immediate before getting it?

pengfei added inline comments.Jun 10 2021, 10:35 PM
llvm/lib/Target/X86/X86InstrInfo.cpp
6358

But CMP64mi32 shouldn't go to here.

It would be nice to have a test, but this change seems ok.

llvm/lib/Target/X86/X86InstrInfo.cpp
6358

The CMPmi32 will be expanded to a load and CMPri32. This switches use the opcode after it's expanded.

pengfei accepted this revision.Jun 11 2021, 1:53 AM

LGTM but let's wait one or two days to see opinions from others.

llvm/lib/Target/X86/X86InstrInfo.cpp
6358

Thanks Craig.
Discussed with Yuanke offline, I understand the fix here is to handle relocImm, i.e., @a in the example.
I think it makes sense to skip it here since we don't know it's value at this point.

This revision is now accepted and ready to land.Jun 11 2021, 1:53 AM

It isn't immediately obvious to me how that can happen.
I don't think this should land without a test.

Maybe we can use an MIR test?

I don't what Intel's original failure looked like, but here's a test that should reproduce this with -run-pass=machinelicm https://reviews.llvm.org/P8267 needs more cleanup.

I hacked the MIR just before machinelicm by sinking the CMP64mi32 and SETCCr into the loop. That makes MachineLICM want to unfold it since the load part is invariant being from a constant global.

LuoYuanke updated this revision to Diff 351624.Jun 11 2021, 7:55 PM

Apply Craig's test case. Many thanks to Craig. :)

I don't what Intel's original failure looked like, but here's a test that should reproduce this with -run-pass=machinelicm https://reviews.llvm.org/P8267 needs more cleanup.

I hacked the MIR just before machinelicm by sinking the CMP64mi32 and SETCCr into the loop. That makes MachineLICM want to unfold it since the load part is invariant being from a constant global.

Hi Craig,

Awesome. Many thanks for the test case. I applied your test case to this patch.

This revision was automatically updated to reflect the committed changes.
LuoYuanke reopened this revision.Jun 13 2021, 12:37 AM
This revision is now accepted and ready to land.Jun 13 2021, 12:37 AM

Update test case to pass expensive check.

This revision was automatically updated to reflect the committed changes.