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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon 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)) ||
----------------
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.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1820
+  case Instruction::FDiv:
+    return I->getOperand(1) == I;
+  case Instruction::Shl:
----------------
Shouldn't this be I->getOperand(1) == OpI ?


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


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1842
+           (!isa<LoadInst>(I->getOperand(0)) ||
+            !isa<LoadInst>(I->getOperand(1)) || I->getOperand(1) == OpI);
+  }
----------------
Why have you only put the constant condition on FADD/FMUL ?


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


Repository:
  rL LLVM

https://reviews.llvm.org/D42981