[PATCH] D92933: [ARM][MachineOutliner] Fix costs model.

Fri Dec 11 01:18:25 PST 2020

samparker added inline comments.

================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:5823
+      // outline this by saving all of its bytes.
+      else
+        NumBytesNoStackCalls += SequenceSize;
----------------
yroux wrote:
> samparker wrote:
> > We were returning here previously, and now it's possible that there's a Candidate that hasn't been inserted into CandidatesWithoutStackFixups. When we update RepeatedSequences on line 5832, are we guaranteed to have not missed some candidates?
> OK, in fact what I tried to do here is just to fix the costs given to the candidates which was wrongly set to Calldefault for all candidates when SP wasn't used and at least one of them was called with save/restore of LR into the stack.
> 
> We were returning here without outling these candidates, if the need of stack fixup was detected, but that wasn't necessary the instructions which migh need such a fixup were marked as illegal for outlining in the code lines 6055-6070
> 
> So, here we are just preparing the next step which will handle the fixups, it is the same logic which is used in the AArch64 implementation.
> 
> I'll renane and change the description of this revision
Okay, thanks for reminding me how this plugs together. So I'm sorry if I'm being dense... but couldn't this still mean we could outline an instruction that is dependent upon one that isn't outlined? If we're iterating through a sequence of instructions, effectively copying them back into RepeatedSequenceLocs, what is stopping us from skipping over an instruction which a following instruction maybe dependent upon?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92933/new/

https://reviews.llvm.org/D92933