[PATCH] D25914: Redo store splitting in CodeGenPrepare

[Wei Mi via llvm-commits]

wmi added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5311-5312
+  // one use.
+  Value *LValue = nullptr;
+  Value *HValue = nullptr;
+  if (!match(SI.getValueOperand(),
----------------
chandlerc wrote:
> No need to initialize these. Leaving them uninitialized makes it easier for MSan to find bugs if we try to use them despite the match failing. (Or a bug in the match that fails to set them.)
Ok, fixed.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5342-5350
+  // If LValue/HValue is a bitcast in another BB and has only one use, move
+  // it to current BB so it may be merged with the splitted stores by dag
+  // combiner.
+  BitCastInst *LBC = dyn_cast<BitCastInst>(LValue);
+  BitCastInst *HBC = dyn_cast<BitCastInst>(HValue);
+  if (LBC && LBC->hasOneUse() && LBC->getParent() != SI.getParent())
+    LValue = Builder.CreateBitCast(LBC->getOperand(0), LBC->getType());
----------------
chandlerc wrote:
> What about just walking LValue and HValue back across any bitcast instructions? That should make it easier to get the type for the cost query, and then make this simpler as you can directly store the pre-bitcast value.
> 
> I also wouldn't worry about how many uses of the bitcast there are either, as bitcasts are expected to be free. As one example, if there are 3 uses of the bitcast, all to do merged stores, we should be willing to unmerge all three of them.
I am not very sure I understand correctly what you mean here, but I remove the hasOneUse limitation. Please take a look whether it looks the same as what is in your mind.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5352
+
+  Type *Ty = Type::getIntNTy(SI.getContext(), HalfValBitSize);
+  Type *PtrTy = Ty->getPointerTo(SI.getPointerAddressSpace());
----------------
chandlerc wrote:
> Just use the type of the input value rather than re-computing?
> 
> Also, you can probably just use a lambda to create the store and then call it for each of the inputs?
Input can be i16 and it is extended to i64 before the "shl + or" bitwise operations. The type of splitted store here is i32. So we cannot necessarily get the i32 from input type. 

I created a lambda and called it for each input. It looks better. 




Repository:
  rL LLVM

https://reviews.llvm.org/D25914