This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Define ComplexPatternFuncMutatesDAG
ClosedPublic

Authored by dmgreen on Aug 4 2021, 9:26 AM.

Details

Summary

Some of the Arm complex pattern functions call canExtractShiftFromMul, which can modify the DAG in-place. For this to be valid and handled successfully we need to define ComplexPatternFuncMutatesDAG.

The test case is a little big, but I didn't have a lot of luck reducing it further.

Diff Detail

Event Timeline

dmgreen created this revision.Aug 4 2021, 9:26 AM
dmgreen requested review of this revision.Aug 4 2021, 9:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2021, 9:27 AM
SjoerdMeijer added inline comments.Aug 5 2021, 2:06 AM
llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp
83

Nice find. Bit of an "obscure" hook, doesn't seem to be documented very well. But I guess it will eventually call canExtractShiftFromMul, perhaps you can clarify that a little bit in the comment.

llvm/test/CodeGen/ARM/shifter_operand.ll
360

You already mentioned the test case being a bit big, but do we need a loop? I guess you do, but that's unclear to me.

If it can't be reduced, can you precommit the test? Then we can see the diff here.

dmgreen updated this revision to Diff 364786.Aug 6 2021, 6:59 AM

Update comment and formatting.

llvm/test/CodeGen/ARM/shifter_operand.ll
360

Unfortunately it crashes, so no changes can be shown and it can't be precommitted. I've not been able to reduce it much beyond this. The smaller example above is doing the same thing - you can see it's turning a mul into a smaller mul and a shift- but not hitting the same assert with deleted nodes.
It's not too big though, all things considered. Some of the other examples I have for other work I haven't been able to reduce past a few thousand instructions, only happening when certain register allocation patterns occur.

SjoerdMeijer accepted this revision.Aug 6 2021, 7:20 AM
SjoerdMeijer added inline comments.
llvm/test/CodeGen/ARM/shifter_operand.ll
360

Alright, cheers, LGTM.

This revision is now accepted and ready to land.Aug 6 2021, 7:20 AM
This revision was automatically updated to reflect the committed changes.
dim added subscribers: emaste, dim.Dec 23 2021, 7:54 AM

David, in this particular change, does "valid and handled successfully" mean "not crash or assert"? I just ran into the following assertion when building part of llvm (13.0.0) itself under FreeBSD 12:

Assertion failed: (NodeToMatch->getOpcode() != ISD::DELETED_NODE && "NodeToMatch was removed partway through selection"), function SelectCodeCommon, file /home/dim/src/llvm/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp, line 3573.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: /home/dim/ins/llvmorg-14-init-00920-gf88ad8d00f9/bin/clang -cc1 -triple armv6kz-unknown-freebsd12.3-gnueabihf -S --mrelax-relocations -disable-free -disable-llvm-verifier -discard-value-names -mrelocation-model static -mconstructor-aliases -target-cpu arm1176jzf-s -target-feature +vfp2 -target-feature +vfp2sp -target-feature -vfp3 -target-feature -vfp3d16 -target-feature -vfp3d16sp -target-feature -vfp3sp -target-feature -fp16 -target-feature -vfp4 -target-feature -vfp4d16 -target-feature -vfp4d16sp -target-feature -vfp4sp -target-feature -fp-armv8 -target-feature -fp-armv8d16 -target-feature -fp-armv8d16sp -target-feature -fp-armv8sp -target-feature -fullfp16 -target-feature +fp64 -target-feature -d32 -target-feature -neon -target-feature -sha2 -target-feature -aes -target-feature -fp16fml -target-feature +strict-align -target-abi aapcs-linux -mfloat-abi hard -fallow-half-arguments-and-returns -ffunction-sections -fdata-sections -O1 -std=c++14 -fdeprecated-macro -fno-rtti -fno-signed-char -faddrsig -fexperimental-new-pass-manager PPCISelLowering-009095.ii
1.      <eof> parser at end of file
2.      Code generation
3.      Running pass 'Function Pass Manager' on module 'PPCISelLowering-009095.cpp'.
4.      Running pass 'ARM Instruction Selection' on function '@_ZN4llvm17PPCTargetLoweringC2ERKNS_16PPCTargetMachineERKNS_12PPCSubtargetE'

And bisecting for the commit that fixed this assertion, I ended up here (and at rG77e8f4eeeeed516a1c79365a4b8128da463d96c4 obviously). Interestingly it crashes during the compilation of the PowerPC instruction selection sources, but when it is targeting ARMv6.

Similar to yours this test case is rather big, namely the preprocessed PPCISelLowering.cpp file, but I'm reducing it too. I would like to see if the end result looks similar to your case here.

dim added a comment.Dec 23 2021, 12:14 PM

Okay, in my case the test case is quite a bit shorter:

; ModuleID = 'bugpoint-reduced-simplified.bc'
source_filename = "PPCISelLowering-min.cpp"
target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
target triple = "armv6-unknown-unknown"

%0 = type { %1, [3 x i8] }
%1 = type { %2 }
%2 = type <{ %3, i8, i8, [179 x [179 x i16]], [179 x [179 x i8]] }>
%3 = type { i32 }
%4 = type <{ %3, i8, i8, [179 x [179 x i16]], [179 x [179 x i8]], [3 x i8] }>

define dso_local arm_aapcscc void @_ZN1jC2Ev(%0* %0) unnamed_addr #0 align 2 {
  %2 = bitcast %0* %0 to %4*
  br i1 undef, label %3, label %4

3:                                                ; preds = %1
  ret void

4:                                                ; preds = %4, %1
  %5 = call arm_aapcscc i32 @_ZN1bIN1g2aiEEdeEv() #1
  %6 = getelementptr inbounds %4, %4* %2, i32 0, i32 3, i32 %5, i32 0
  store i16 0, i16* %6, align 2
  %7 = getelementptr inbounds %4, %4* %2, i32 0, i32 4, i32 %5, i32 0
  store i8 0, i8* %7, align 1
  br label %4, !llvm.loop !1
}

declare arm_aapcscc i32 @_ZN1bIN1g2aiEEdeEv() local_unnamed_addr #0

attributes #0 = { "target-features"="+armv6,-thumb-mode" }
attributes #1 = { nounwind }

!llvm.ident = !{!0}

!0 = !{!"FreeBSD clang version 13.0.0 (git@github.com:llvm/llvm-project.git llvmorg-13.0.0-0-gd7b669b3a303)"}
!1 = distinct !{!1, !2}
!2 = !{!"llvm.loop.unroll.disable"}

OK great. Thanks for the report. llvm 14 should be all OK then?

in this particular change, does "valid and handled successfully" mean "not crash or assert"?

Yep - I think it was some sort of crash IIRC.

https://github.com/llvm/llvm-project/issues/53396, also targeting armv6kz, was fixed by this change, . 13.0.1-rc3 doesn't have the issue thanks to the cherry-pick.

dmgreen added a comment.EditedJan 27 2022, 3:56 AM

Good stuff. I was wondering if it was. Thanks for checking.