[PATCH] D42981: [COST] Fix cost model of load instructions on X86

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1824
+           !isa<LoadInst>(I->getOperand(1)) || I->getOperand(0) == OpI;
+  case Instruction::Mul:
+    return (!isa<LoadInst>(I->getOperand(0)) ||
----------------
RKSimon wrote:
> ABataev wrote:
> > RKSimon wrote:
> > > A little tricky, but we only fold certain operands (Operand(1) typically) so if it doesn't commute then we can't fold - ISD::SUB/FSUB/FDIV/SHL/ASHR/LSHR will all definitely be affected by this.
> > > 
> > > Also we need to handle the case where both operands are loads - we will still have at least one load cost.
> > 1. Agree, missed it. Will be fixed.
> > 2. Yes, it is handled already. The additional checks here are exactly to check this situation.
> Shift instructions can only fold the second operand as well (and even then very poorly....). I'd be tempted to just not include them in isFreeOp at all tbh.
No, actually only the first operand can be the memory address and, thus, can be folded


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1820
+  case Instruction::FDiv:
+    return I->getOperand(1) == I;
+  case Instruction::Shl:
----------------
RKSimon wrote:
> Shouldn't this be I->getOperand(1) == OpI ?
Yes, missed this when updated the patch last time, thanks!


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1836
+            !isa<LoadInst>(I->getOperand(1)) || I->getOperand(1) == OpI) &&
+           Ty->getScalarSizeInBits() <= 64;
+  case Instruction::FAdd:
----------------
RKSimon wrote:
> Please can you add a comment explaining the reason for the ScalarSIzeInBits condition?
Yes, sure.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1842
+           (!isa<LoadInst>(I->getOperand(0)) ||
+            !isa<LoadInst>(I->getOperand(1)) || I->getOperand(1) == OpI);
+  }
----------------
RKSimon wrote:
> Why have you only put the constant condition on FADD/FMUL ?
Integer operations allow using register/immediate value as one of the operands.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1886
+                           UI->getOperand(0)->getType(), UI) == 0 &&
+          isFreeOp(UI->user_back(), UI->getType(), UI))
+        return 0;
----------------
RKSimon wrote:
> Comment this please.
Ok.


Repository:
  rL LLVM

https://reviews.llvm.org/D42981