[PATCH] D126137: [X86] Add support for `-mharden-sls=all`

[Phoebe Wang via Phabricator via cfe-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:346
+      auto IsIndirectTailCall = [I, &Desc]() {
+        return Desc.isCall() && Desc.isReturn() && Desc.isBarrier() &&
+               !I->getOperand(0).isGlobal();
----------------
nickdesaulniers wrote:
> Why `isBarrier`? Maybe add a comment that mentions a specific TableGen record from llvm/lib/Target/X86/X86InstrControl.td?  That would help me verify that `isBarrier()` is correct here.
> 
> I'm guessing this is:
> 
> 363 let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1,                  
> 364     isCodeGenOnly = 1, Uses = [RSP, SSP] in {
> 
> block in llvm/lib/Target/X86/X86InstrControl.td.
> 
> Otherwise llvm/lib/Target/X86/X86FrameLowering.cpp has a static function that I think is precisely what we want here, called ` isTailCallOpcode`. Perhaps move that to a header shared between the two translation units then reuse it here?  A few other backends have an `IsTailCall` method though they're inconsistent where they define it. It still would be good though to try to match existing conventions which makes jumping between backends easier.
> Why `isBarrier`?

Because we have conditional tail calls, see defination of `TCRETURNdicc`.

> Perhaps move that to a header shared between the two translation units then reuse it here?

We can't. They are not overlapped.

> A few other backends have an `IsTailCall` method though they're inconsistent where they define it

`IsTailCall` is an attribute of call instruction in IR. We lost it in MIR phase.


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:347
+        return Desc.isCall() && Desc.isReturn() && Desc.isBarrier() &&
+               !I->getOperand(0).isGlobal();
+      };
----------------
nickdesaulniers wrote:
> Rather than `!...isGlobal()`, would it be more precise to use `getOperand(0).isReg()`?  Looking at all of the values of `enum MachineOperandType`, I think it would be more precise if we check the operand is of one type, rather than "not one type."
OK, check opcode instead.


================
Comment at: llvm/test/CodeGen/X86/speculation-hardening-sls.ll:7
+; CHECK:         jle
+; CEHCK-NOT:     int3
+; CHECK:         retq
----------------
nickdesaulniers wrote:
> typo
Good catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126137