[PATCH] D152407: [AArch64] Merge LDRSWpre-LD[U]RSW pair into LDPSWpre.

Wed Jul 26 14:25:47 PDT 2023

chaosdefinition reopened this revision.
chaosdefinition added inline comments.
This revision is now accepted and ready to land.

================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1334
   if (NonSExtOpc == getMatchingNonSExtOpcode(OpcB, &PairIsValidLdStrOpc)) {
     Flags.setSExtIdx(NonSExtOpc == (unsigned)OpcA ? 1 : 0);
     return true;
----------------
>>! In D152407#4534575, @asmok-g wrote:
> 
> Hi there! I'm not an expert here, but speaking with @scw and he thinks the fix doesn't address the "sxtw on wrong register bug". He'll probably be able to comment with details later but his vote is to rollback for now. Can you please do ?
> 
> Thanks

The wrong register in `SBFMXri` was resulted from `SExtIdx` being set to 0 here.  However, to reach here:
- `OpcA` and `OpcB` must be different;
- `OpcA` and `OpcB` must have the same `NonSExtOpcode`.
So this code path works only for pairs of `LDRSWui`-`LDRWui` and `LDURSWi`-`LDURWi` before.

Now with this patch, the only added case to reach here is `LDRSWpre`-`LDRWpre` pairs, as was caught by the miscompilation.  So my quick fix should prevent such pairs from generating `SBFMXri`, by bailing out early on two pre-indexed loads.

I guess there might be a better way to fix this.  Any thoughts while I work on an updated patch?

Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152407