[PATCH] [AArch64] Improve and enable the SeparateConstOffsetFromGEP for AArch64 backend.

Hao Liu Hao.Liu at arm.com
Mon Nov 10 22:30:20 PST 2014


>>! In D5864#39, @jingyue wrote:
> I see what's going on: you only extract constant offsets from array indices, and leave struct indices in those transformXXX functions. Thanks for clarification! I think that's worth some comment though, and I will point that out in my inline comments. 

Hi Jingyue,

Thanks for your detailed comments, which is really very helpful. I've refactored the patch and attached.
Review please.

Thanks,
-Hao

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:721
@@ +720,3 @@
+  for (unsigned I = 1, E = GEP->getNumOperands(); I != E; ++I, ++GTI) {
+    if (!isa<ConstantInt>(GEP->getOperand(I))) {
+      Value *Idx = GEP->getOperand(I);
----------------
jingyue wrote:
> Why checking whether it's a constant? Shouldn't you check whether it is an array index? My understanding is, the offsets of structure indices (which are guaranteed to be constant) and the constant offsets of array indices are already counted in accumulativeByteOffsets, and now you only need to rebuild the variadic part of the array indices. 
Yes, you are right. I'll add comments to explain this.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:827
@@ +826,3 @@
+  // splitting probably won't be beneficial.
+  if (!LowerGEP) {
+    TargetTransformInfo &TTI = getAnalysis<TargetTransformInfo>();
----------------
jingyue wrote:
> Why do we need to check LowerGEP here? My understanding is, even when LowerGEP is set, +AccumulativeByteOffset will still become part of the lowered GEPs or arithmetics (see how transformToSimplerGEPs create a GEP with offset AccumulativeByteOffset, and how transformToArithmetics create an add with offset AccumulativeByteOffset). Shouldn't we still check whether this offset can be nicely folded into an addressing mode? 
I think even a pointer plus a constant index can not be matched in an addressing mode, it is worth to lower a GEP with multiple indices. Because we can do optimizations on the variable indices. Also if the constant index is extracted from multiple indices, the result looks better than the original GEP.
If I remove such check, some test cases will fail. So it is kept when disable LowerGEP.

http://reviews.llvm.org/D5864






More information about the llvm-commits mailing list