Page MenuHomePhabricator

[x86] Relax the check in areLoadsFromSameBasePtr

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



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


Event Timeline

eraman created this revision.Apr 7 2017, 5:02 PM
mkuper edited edge metadata.

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

craig.topper edited edge metadata.Apr 10 2017, 3:22 PM

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.

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() ?


Crashes can be fixed if this logic is included.