[x86] Relax the check in areLoadsFromSameBasePtr
ClosedPublic

Authored by eraman on Apr 7 2017, 5:02 PM.

Details

Summary

The code returns false if the Scale value (which should match in Load1 and Load2) is not 1. I think all that is required is every operand except the displacement must be identical.

Diff Detail

Repository
rL LLVM
eraman created this revision.Apr 7 2017, 5:02 PM

This makes sense to me, but I don't understand why the Scale == 1 requirement was there to begin with.
+ctopper for another look.

Evan Cheng added this in r94147 at that time there was an additional check on the index register itself too. This is what the "Index should be Reg0" comment was referring too, but I don't know what that means. Its before my time. Jakob Stoklund Olesen removed that check in r100497, but did not update the comment about Reg0.

I think this change is fine.

While you're touching this can you use the X86::AddrBaseReg, X86::AddrScaleAmt, etc. constants when calling getOperand. This will improve readability here.

eraman updated this revision to Diff 94762.Apr 10 2017, 5:52 PM

Used getOperand(X86::AddrBaseReg) etc instead of getOperand(0). Also removed if the chain operands match. This check shouldn't be in areLoadsFromSameBasePtr and the caller already passes two loads that are uses of the same chain node.

This revision is now accepted and ready to land.Apr 11 2017, 1:21 PM
This revision was automatically updated to reflect the committed changes.

Seems it causes crash in stage2.
http://bb.pgr.jp/builders/clang-3stage-x86_64-linux/builds/14942

As far as I investigated, this change misses the case "Load1->getOperand(5) != Load2->getOperand(5)"

t69: i64,ch = <<Unknown Machine Node #63812>><Mem:LD8[%_M_p.i.i.i.i.i34](tbaa=<0xa62bec8>)> t2, TargetConstant:i8<1>, Register:i64 %noreg, TargetConstant:i32<0>, Register:i32 %noreg, t2:1
t78: i64,ch = <<Unknown Machine Node #63812>><Mem:LD8[%9](tbaa=<0xa55faf8>)> t2, TargetConstant:i8<1>, Register:i64 %noreg, TargetConstant:i32<56>, Register:i32 %noreg, t29:1

Is it true for areLoadsFromSameBasePtr() ?

llvm/trunk/lib/Target/X86/X86InstrInfo.cpp
8985

Crashes can be fixed if this logic is included.