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

[Philip Reames via Phabricator via llvm-commits]

reames marked 2 inline comments as done.
reames added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:25999
+      Chain};
+    SDNode *Res = DAG.getMachineNode(X86::LOCK_OR64mi32, DL, MVT::Other, Ops);
+    return SDValue(Res, 0);
----------------
craig.topper wrote:
> reames wrote:
> > craig.topper wrote:
> > > Shouldn't this be mi8 for a shorter encoding?
> > > 
> > > Does 64-bit really need a 64-bit access or can we just use a 32-bit access?
> > 
> > For the 64 bit, we need a valid stack location.  There's no guarantee that ESP will contain a valid address.  (Since it's the 32 bit truncation of RSP and the stack may be outside the low 4 GB.)
> Sorry. I meant can we use OR32mi8Locked in 64-bit mode, but still use RSP for the address ? The amount of stack space accessed isn't important right? We could read/write 4 bytes from the stack? I mainly ask because OR32mi8Locked is 1 byte shorter to encode than LOCK_OR64mi8 if none of the registers used force the use of a REX prefix.
Ah, gotcha.  When I do the next rev, I'll make this change.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26255
+
+  bool HasRedZone = Subtarget.isTargetLinux() && Subtarget.isTarget64BitLP64(); 
+  int SPOffset = HasRedZone ? -8 : 0;
----------------
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.


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

https://reviews.llvm.org/D58632