Fix backward operands in call to isTruncateFree() and improve comments.

[srking]

Summary

In at least one case, DAGCombiner calls isTruncateFree() with operands in reverse order. Fixed that and improved comments to make the direction and corner case behavior more obvious.

[srking]

Oops, patch is in wrong direction. Will upload new patch.

[srking]

Fixed patch file direction.

[srking]

Removed the assert() due to cases that call isTruncateFree without regard to type size. Improved comment for these odd cases.
Verified regression checks pass.

[arsenm]

Do you have a testcase for this?

[jroelofs]

include/llvm/Target/TargetLowering.h

1568–1575

Please fix this one...

1579

... and this one too while you're here.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

9725–9726

Drop this comment, and don't change the formatting of the following lines.

[jroelofs]

include/llvm/Target/TargetLowering.h

1527–1528

Would be nice to fix this one too.

[qcolombet]

Hi,

The fix LGTM, but do you have a test case exposing the problem?

Thanks,
-Quentin

[srking]

Tried to address review comments from Jonathan Roelofs. NFC from previous patch. Updated patch title text and summary to remove statement about asserts.

[jroelofs]

thanks!

include/llvm/Target/TargetLowering.h

1565–1566

s/Ty2/FromTy/ s/Ty1/ToTy/

1567–1568

s/Ty2/FromTy/ s/Ty1/ToTy/

[srking]

In D8304#213632, @arsenm wrote:

Do you have a testcase for this?

Unfortunately, I do not see a succinct way to test for this. The bug is buried in a cost analysis algorithm. The problem was detected by investigating nonsense truncation cases, i.e. where From < To. By inspection, we see the original code places the destination as "From" and the operand as "To" in the truncation check.

I verified all "make check" tests pass with the patch.

Thanks,
-steve

[srking]

include/llvm/Target/TargetLowering.h

1565–1566

rather s/Ty1/FromTy/ s/Ty2/ToTy/

1567–1568

I understand x86 behavior here, but I can't understand what the original comment is trying to say. Taking your word for it.

[srking]

Fix more old style comments caught by Jonathan. NFC.
The meaning of one of the original comments is mysterious. This patch does not clarify it.

[jroelofs]

include/llvm/Target/TargetLowering.h

1567–1575

Now that I've read it again, I don't get what it's trying to say either... I can kind of see it making sense if the operands were accidentally swapped.

[srking]

include/llvm/Target/TargetLowering.h

1567–1575

Ya, I tried for a while to assimilate this one. Confusing comments are often worse than nothing. If you aprove, I will update the patch to delete this comment.

[hfinkel]

This LGTM. If you have a test case, great, otherwise you can commit without one.

include/llvm/Target/TargetLowering.h

1567–1575

Exactly what would you want to delete? Perhaps we should just reword this to say that the function should return true when it is likely that the truncate can be freely folded with an instruction defining a value of FromTy. If the defining instruction is unknown (because you're looking at a function argument, PHI, etc.) then you may need an explicit truncate, and that's not necessarily free (but this function does not deal with those cases).

[srking]

Updated with improved comment from Hal Finkel.

[srking]

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@245385 91177308-0d34-0410-b5e6-96231b3b80d8

include/llvm/Target/TargetLowering.h

public:
   /// Return true if it's significantly cheaper to shift a vector by a uniform
   /// scalar than by an amount which will vary across each lane. On x86, for
   /// example, there is a "psllw" instruction for the former case, but no simple
   /// instruction for a general "a << b" operation on vectors.
   virtual bool isVectorShiftByScalarCheap(Type *Ty) const {
     return false;
   }
 
-  /// Return true if it's free to truncate a value of type Ty1 to type
-  /// Ty2. e.g. On x86 it's free to truncate a i32 value in register EAX to i16
+  /// Return true if it's free to truncate a value of type FromTy to type
+  /// ToTy. e.g. On x86 it's free to truncate a i32 value in register EAX to i16
   /// by referencing its sub-register AX.
-  virtual bool isTruncateFree(Type * /*Ty1*/, Type * /*Ty2*/) const {
+  /// Targets must return false when FromTy <= ToTy.
+  virtual bool isTruncateFree(Type *FromTy, Type *ToTy) const {
     return false;
   }
 
-  /// Return true if a truncation from Ty1 to Ty2 is permitted when deciding
+  /// Return true if a truncation from FromTy to ToTy is permitted when deciding
   /// whether a call is in tail position. Typically this means that both results
   /// would be assigned to the same register or stack slot, but it could mean
   /// the target performs adequate checks of its own before proceeding with the
-  /// tail call.
-  virtual bool allowTruncateForTailCall(Type * /*Ty1*/, Type * /*Ty2*/) const {
+  /// tail call.  Targets must return false when FromTy <= ToTy.
+  virtual bool allowTruncateForTailCall(Type *FromTy, Type *ToTy) const {
     return false;
   }
 
-  virtual bool isTruncateFree(EVT /*VT1*/, EVT /*VT2*/) const {
+  virtual bool isTruncateFree(EVT FromVT, EVT ToVT) const {
     return false;
   }
 
   virtual bool isProfitableToHoist(Instruction *I) const { return true; }
 
   /// Return true if the extension represented by \p I is free.
   /// Unlikely the is[Z|FP]ExtFree family which is based on types,
   /// this method can use the context provided by \p I to decide
bool isExtFree(const Instruction *I) const {
     case Instruction::SExt:
       break;
     default:
       llvm_unreachable("Instruction is not an extension");
     }
     return isExtFreeImpl(I);
   }
 
-  /// Return true if any actual instruction that defines a value of type Ty1
-  /// implicitly zero-extends the value to Ty2 in the result register.
+  /// Return true if any actual instruction that defines a value of type FromTy
+  /// implicitly zero-extends the value to ToTy in the result register.
   ///
-  /// This does not necessarily include registers defined in unknown ways, such
-  /// as incoming arguments, or copies from unknown virtual registers. Also, if
-  /// isTruncateFree(Ty2, Ty1) is true, this does not necessarily apply to
-  /// truncate instructions. e.g. on x86-64, all instructions that define 32-bit
-  /// values implicit zero-extend the result out to 64 bits.
-  virtual bool isZExtFree(Type * /*Ty1*/, Type * /*Ty2*/) const {
+  /// The function should return true when it is likely that the truncate can
+  /// be freely folded with an instruction defining a value of FromTy. If
+  /// the defining instruction is unknown (because you're looking at a
+  /// function argument, PHI, etc.) then the target may require an
+  /// explicit truncate, which is not necessarily free, but this function
+  /// does not deal with those cases.
+  /// Targets must return false when FromTy >= ToTy.
+  virtual bool isZExtFree(Type *FromTy, Type *ToTy) const {
     return false;
   }
 
-  virtual bool isZExtFree(EVT /*VT1*/, EVT /*VT2*/) const {
+  virtual bool isZExtFree(EVT FromTy, EVT ToTy) const {
     return false;
   }
 
   /// Return true if the target supplies and combines to a paired load
   /// two loaded values of type LoadedType next to each other in memory.
   /// RequiredAlignment gives the minimal alignment constraints that must be met
   /// to be able to select this paired load.
   ///

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

struct Cost {
 
     /// \brief Account for slicing gain in the current cost.
     /// Slicing provide a few gains like removing a shift or a
     /// truncate. This method allows to grow the cost of the original
     /// load with the gain from this slice.
     void addSliceGain(const LoadedSlice &LS) {
       // Each slice saves a truncate.
       const TargetLowering &TLI = LS.DAG->getTargetLoweringInfo();
-      if (!TLI.isTruncateFree(LS.Inst->getValueType(0),
-                              LS.Inst->getOperand(0).getValueType()))
+      if (!TLI.isTruncateFree(LS.Inst->getOperand(0).getValueType(),
+                              LS.Inst->getValueType(0)))
         ++Truncates;
       // If there is a shift amount, this slice gets rid of it.
       if (LS.Shift)
         ++Shift;
       // If this slice can merge a cross register bank copy, account for it.
       if (LS.canMergeExpensiveCrossRegisterBankCopy())
         ++CrossRegisterBanksCopies;
     }