[PATCH] D35049: LSR tunings for SystemZ, with some minor common code changes

Fri Jul 21 01:50:22 PDT 2017

jonpa updated this revision to Diff 107638.
jonpa added a comment.

SystemZ part updated per review.

> I'm now wondering what the difference between isLegalAddressingMode and isFoldableMemAccessOffset is, now that they both take a specific instruction. Don't these two functions now answer the exact same question? And if so, shouldn't the implementation then be merged? The following questions about differences between the two would be moot if we actually had a shared implementation here.

I agree this isn't perfect. I also noticed this before, and thought this could be possibly refactored but however also noticed the many different overloaded versions of isLegalAddressingMode() and thought that it could wait as a next step, perhaps. RateFormula() currently only checks the fixup instructions for the offsets, so I am not sure this is a trivial change.

> For example, you change isLegalAddressingMode to check for hasNoIndexReg, but not needsD12. If the function does get an Instruction, and we can see that it won't accept large displacements, shouldn't we then reject the address from isLegalAddressingMode as well?

Makes sense -- see below.

> Also, the various checks for float/vector access in isFoldableMemAccessOffset -- shouldn't they move to hasLessAddressing? We know that float/vector accesses won't accept large displacements, so shouldn't the function say so?

Done.

> The comment before hasLessAddressing talks about accesses being converted to vector instructions, but that happens only on z13 or later. Shouldn't this then be guarded by a Subtarget feature check just like the existing code in isFoldableMemAccessOffset does?

Good point - fixed.

> Finally, I'm not really happy about the names ... hasLessAddressing sound a bit strange. Maybe instead a function called "supportedAddressingMode" that takes an Instruction and returns a description of the addressing mode (long vs. short displacement, index vs. no index), either as an enum or as a pair of booleans.

Changed it and agree this is more clear by returning both values as a pair. Does it look acceptable now?

I see then two points that could be handled in separate patches:

- Model the cost of live-out PHIs. I actually see now that this is supposed to be handled with isExistingPhi(), but I guess could always try to look further why it appeared to not always work for me.
- Try if possible to merge isLegalAddressingMode() with isFoldableMemAccessOffset() and also improve isLegalAddressingMode() to check the offset. I have inserted a TODO comment so that we remember this.

https://reviews.llvm.org/D35049

Files:
  include/llvm/Analysis/TargetTransformInfo.h
  include/llvm/Analysis/TargetTransformInfoImpl.h
  include/llvm/CodeGen/BasicTTIImpl.h
  include/llvm/Target/TargetLowering.h
  lib/Analysis/TargetTransformInfo.cpp
  lib/CodeGen/TargetLoweringBase.cpp
  lib/Target/AArch64/AArch64ISelLowering.cpp
  lib/Target/AArch64/AArch64ISelLowering.h
  lib/Target/AMDGPU/SIISelLowering.cpp
  lib/Target/AMDGPU/SIISelLowering.h
  lib/Target/ARM/ARMISelLowering.cpp
  lib/Target/ARM/ARMISelLowering.h
  lib/Target/AVR/AVRISelLowering.cpp
  lib/Target/AVR/AVRISelLowering.h
  lib/Target/Hexagon/HexagonISelLowering.cpp
  lib/Target/Hexagon/HexagonISelLowering.h
  lib/Target/Mips/MipsISelLowering.cpp
  lib/Target/Mips/MipsISelLowering.h
  lib/Target/NVPTX/NVPTXISelLowering.cpp
  lib/Target/NVPTX/NVPTXISelLowering.h
  lib/Target/PowerPC/PPCISelLowering.cpp
  lib/Target/PowerPC/PPCISelLowering.h
  lib/Target/SystemZ/SystemZISelLowering.cpp
  lib/Target/SystemZ/SystemZISelLowering.h
  lib/Target/SystemZ/SystemZTargetTransformInfo.cpp
  lib/Target/SystemZ/SystemZTargetTransformInfo.h
  lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
  lib/Target/WebAssembly/WebAssemblyISelLowering.h
  lib/Target/X86/X86ISelLowering.cpp
  lib/Target/X86/X86ISelLowering.h
  lib/Target/XCore/XCoreISelLowering.cpp
  lib/Target/XCore/XCoreISelLowering.h
  lib/Transforms/Scalar/LoopStrengthReduce.cpp
  test/CodeGen/SystemZ/dag-combine-01.ll
  test/CodeGen/SystemZ/loop-01.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D35049.107638.patch
Type: text/x-patch
Size: 39498 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170721/38a03419/attachment.bin>