[PATCH] D58632: [X86] Improve lowering of idemptotent RMW operations

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26255
+
+  bool HasRedZone = Subtarget.isTargetLinux() && Subtarget.isTarget64BitLP64(); 
+  int SPOffset = HasRedZone ? -8 : 0;
----------------
reames wrote:
> jfb wrote:
> > craig.topper wrote:
> > > jfb wrote:
> > > > I'd rather see something more precise here.
> > > > 
> > > > We have a `noredzone` function attribute which should be honored:
> > > > https://releases.llvm.org/3.7.0/docs/LangRef.html#id630
> > > > 
> > > > aarch64 has some handling for redzone too:
> > > > https://llvm.org/doxygen/AArch64FrameLowering_8cpp_source.html#l00186
> > > > 
> > > > So does X86FrameLowering.cpp, X86InstrInfo.cpp, and X86MachineFunctionInfo.h.
> > > > 
> > > > Some other architectures to this too. I think you should do the same: if the attribute isn't present, redzone away. If some targets don't have a redzone yet don't set the attribute, they've already opened themselves to pain. Maybe we need a PSA on the mailing list to make sure these targets set `noredzone` on all their functions.
> > > But clang doesn't currently do that does it? Wouldn't it also create a bitcode backwards compatibility issue?
> > The above heuristic must, at a minimum, be updated to obey `noredzone`.
> > 
> > I'm saying that on top of this I think we want something more general. Yes there's potential bugs lurking and incompatibilities, hence why I think a PSA is warranted, I think the discussion will be helpful.
> Ok, I think this is snowballing a bit.  From a quick dig through the code, the existing redzone code is a mix of "have we relied on the redzone", and "are we allowed to use the redzone".  I'm fine trying to do a bit of cleanup there, but I would *very* much like to separate that into a distinct set of patches.
> 
> One point, I don't agree with your intepretation of the noredzone attribute.  The LangRef explicit says it only matters if the ABI would otherwise allow the use of the redzone.  
> 
> Can we land this w/o using the redzone?  Banging on top of stack isn't great, but it's a lot better than the mfence.  If everyone is okay with that, I'll rev the patch to remove the offset for the moment.
Works for me (as long as it doesn't special-case any ABI), maybe leave a FIXME to use redzone and check the attribute, etc.


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

https://reviews.llvm.org/D58632