[PATCH] D31081: [ARM] ScheduleDAGRRList::DelayForLiveRegsBottomUp must consider OptionalDefs

[James Molloy via Phabricator via llvm-commits]

jmolloy added a comment.

Hi,

I've spent some time coming up to speed on this in picking it up from Artyom. The million dollar question on this seems to be whether OptionalDefs are a total hack and should be ripped out entirely, or should be kept. I think in the latter case, the patch itself looks correct.

Firstly, while OptionalDef is only used in the ARM backend it is used there pervasively. I saw Eli mention that he'd experimented with multiclassing Thumb1 instructions to get rid of the optionaldefs - but optionaldef isn't just used in Thumb-1, it's used in all ARM-mode instructions and most Thumb-2 instructions too.

I certainly agree that the design of AArch64 is far superior, with separate S and non-S variants of instructions, however I also think that moving the ARM backend to this model is a large project fraught with danger (so has to be well thought out to avoid the strong possibility of a mistranslation).

OptionalDefs also seem to be well-specified - they are simply a def that has a default if none is set. Therefore there should be no bad interactions with the register allocator as Matthias had worried (if there was, we'd have seen this before, it's used so pervasively!)

I'd therefore argue that this patch is correct, fixes a known defect, so should be committed.

Thoughts?

James


https://reviews.llvm.org/D31081