llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.09k stars 11.6k forks source link

DAGCombiner doesn't check legality for merging stores with noimplicitfloat #39641

Open nemanjai opened 5 years ago

nemanjai commented 5 years ago
Bugzilla Link 40294
Version trunk
OS All

Extended Description

This can be reproduced as follows:

$ cat a.ll 
define dso_local void @test(i64* nocapture %arr1) local_unnamed_addr #0 {
entry:
  %arrayidx = getelementptr inbounds i64, i64* %arr1, i64 2
  %0 = load i64, i64* %arrayidx, align 8
  store i64 %0, i64* %arr1, align 8
  %arrayidx2 = getelementptr inbounds i64, i64* %arr1, i64 3
  %1 = load i64, i64* %arrayidx2, align 8
  %arrayidx3 = getelementptr inbounds i64, i64* %arr1, i64 1
  store i64 %1, i64* %arrayidx3, align 8
  ret void
}

attributes #0 = { noimplicitfloat }

$ llc a.ll -mtriple=powerpc64le-unknown-unknown
llc: /home/nemanjai/llvm/llvm-clean/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:970: void {anonymous}::SelectionDAGLegalize::LegalizeOp(llvm::SDNode*): Assertion `(TLI.getTypeAction(*DAG.getContext(), Op.getValueType()) == TargetLowering::TypeLegal || TLI.isTypeLegal(Op.getValueType()) || Op.getOpcode() == ISD::TargetConstant || Op.getOpcode() == ISD::Register) && "Unexpected illegal type!"' failed.

The problem is that PPC does not override TargetLowering::mergeStoresAfterLegalization() and the merging will produce an i128 which isn't legal. It will of course produce it prior to legalization as well, but the legalizer will undo that, the problem is after legalization.

Note that without the noimplicitfloat attribute, things work because using vectors is allowed and then it will find a corresponding vector type.

llvmbot commented 1 year ago

@llvm/issue-subscribers-backend-powerpc

nemanjai commented 1 year ago

This isn't really specific to the PPC back end. The problem is in the legalizer.

arsenm commented 1 year ago

Right but anything like this needs to be connected to a relevant backend. Nobody is going to fix an abstract issue for a combination of legal operations that isn't really used

nemanjai commented 1 year ago

Fair enough. I'll post a patch for this as soon as I get a chance.