[llvm-commits] [llvm] r52254 - in /llvm/trunk: include/llvm/Target/TargetLowering.h lib/CodeGen/SelectionDAG/DAGCombiner.cpp lib/CodeGen/SelectionDAG/LegalizeDAG.cpp lib/Target/X86/X86InstrSSE.td test/CodeGen/X86/2008-06-13-NotVolatileLoadStore.ll test/CodeGen/X86/2008-06-13-VolatileLoadStore.ll

[Duncan Sands]

Hi Dan,

> Do you know if there are any places left where this change
> makes the DAGCombiner more conservative? I'm curious what
> kinds of transformations it was getting away with before.

the remaining places are those that check for legality
of an operation even if !AfterLegalize.

For example, lines 2409 -> 2440 perform the transform

  // fold sra (shl X, m), result_size - n
  // -> (sign_extend (trunc (shl X, result_size - n - m))) for
  // result_size - n != m.

It does the following checks:

          TLI.isOperationLegal(ISD::SIGN_EXTEND, TruncVT) &&
          TLI.isOperationLegal(ISD::TRUNCATE, VT) &&
          TLI.isTruncateFree(VT, TruncVT)) {

These checks used to pass if TruncVT/VT (as appropriate) were
illegal types (dunno about the isTruncateFree check), while they
won't pass for illegal types now.  It's not clear that turning
this on when !AfterLegalize is a good idea, so I left it alone.

Another example is lines 4670 -> 4679, which does:

  // If this is an FP_ROUND or TRUNC followed by a store, fold this into a
  // truncating store.  We can do this even if this is already a truncstore.
  if ((Value.getOpcode() == ISD::FP_ROUND || Value.getOpcode() == ISD::TRUNCATE)
      && Value.Val->hasOneUse() && ST->isUnindexed() &&
      TLI.isTruncStoreLegal(Value.getOperand(0).getValueType(),
                            ST->getMemoryVT())) {
    return DAG.getTruncStore(Chain, Value.getOperand(0), Ptr, ST->getSrcValue(),
                             ST->getSrcValueOffset(), ST->getMemoryVT(),
                             ST->isVolatile(), ST->getAlignment());
  }

Here it is tempting to change the check to:
      (!AfterLegalize || TLI.isTruncStoreLegal(Value.getOperand(0).getValueType(), ST->getMemoryVT()))
because otherwise this is no longer being performed when Value.getOperand(0) has an
illegal type.  However it requires more work than this (as you will see if you do it
and run "make check") because the call to isTruncStoreLegal is also implicitly checking
whether a truncstore is valid at all with these types.

There are also (around lines 2643 and 4057) checks for legality of SELECT_CC and
BR_CC.  If you perform these when !AfterLegalize then you get masses of testsuite
failures.  So I left them alone.

These are the only cases AFAICS.

Ciao,

Duncan.

PS: The logic in DAGCombiner::visitEXTRACT_VECTOR_ELT may be bogus for volatile loads.
I didn't think about it because I don't know much about vectors.  Can you please take
a look.