[PATCH] D124894: Avoid 8 and 16bit switch conditions on x86

[Craig Topper via Phabricator via llvm-commits]

craig.topper accepted this revision.
craig.topper added a comment.
This revision is now accepted and ready to land.

LGTM with what I think is a typo in the X86 override fixed.



================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:1607
+TargetLoweringBase::shouldExtendSwitch(const SwitchInst &SI,
+                                       Instruction::CastOps *ExtOp) const {
+  Value *Cond = SI.getCondition();
----------------
MatzeB wrote:
> craig.topper wrote:
> > I think we would typically pass ExtOp by reference so you can't pass null.
> > I think we would typically pass ExtOp by reference so you can't pass null.
> 
> No longer relevant as the parameter is gone now, but FWIW:
> 
> I prefer pointers for "out parameters" because that makes it clearer when readining the calling code that there is an "out parameter".
> 
> i.e. `foobar(&baz)` has a clear signal that baz is probably an "out parameter", for `foobar(baz)` it's easy to miss this when `baz` happens to be a writable reference.
> 
> I find that more valuable than having an assurance that nobody passes `nullptr`...
Yeah that is an unfortunate annoyance of the syntax. I was basing on what I usually see done in code. Our coding standards probably say nothing about this.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:1614
+  EVT OldVT = getValueType(DL, CondType);
+  MVT RegType = getRegisterType(Context, OldVT);
+  unsigned RegWidth = RegType.getSizeInBits();
----------------
MatzeB wrote:
> MatzeB wrote:
> > craig.topper wrote:
> > > MatzeB wrote:
> > > > MatzeB wrote:
> > > > > craig.topper wrote:
> > > > > > MatzeB wrote:
> > > > > > > craig.topper wrote:
> > > > > > > > If we just gave targets control over RegType here would that be enough?
> > > > > > > The callback is used by `CodeGenPrepare` though which deals with llvm IR and rather has `Type*`s than `MVT`s...
> > > > > > Ok could we return ExtType from the target and use ExtType to calculate RegWidth for `if` on 1617?
> > > > > > 
> > > > > > Realy, I'm wondering why we had to move all of the code into TargetLowering and duplicate Argument attributes checking in X86. Or is there some subtle difference that prevents us from sharing the Argument attribute handling.
> > > > > If you prefer we can share the logic starting at line 1632 (deciding between ZExt/SExt and the argument handling) simplifying the X86 callback with the drawback that targets can no longer opt-out of that logic (admittedly I don't know why they would want to opt-out, so I don't care too deeply).
> > > > Hmm we need an `MVT` if we want to have the `isSExtCheaperThanZExt` in the shared code as there's no equivalent callback for `Type*`...
> > > I guess it might be better to give targets the control. There was a patch proposing to ignore the attributes on X86.  D122963.
> > Oh, I just switched it to a more minimal version of this patch. Should I go back to the old version?
> > There was a patch proposing to ignore the attributes on X86
> 
> FWIW: While working on this I did originally not replicate the logic in the X86 callback and the result was worse at least for the llvm unit-tests.
> 
> I think ultimately we had a global instruction selection and wouldn't need to rely on IR->IR transformations upfront... (because I think for optimal results you want to control the overflow-check independently of the value used for the jump-table address calculations; it also depends about what sorts of extensions and truncations the target gets for free; etc.).
> 
> But yeah I think for the LLVM of today this patch here at least improves some cases.
We can keep the minimal patch. If the x86 patch were to go through and ignore the attribute, it would only be for i8/i16 types. Extending those to i32 isn't free or more costly no matter which we choose.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:33724
+                                              EVT ConditionVT) const {
+  // Avoid 8 and 16 bit types because the increase the chance for unnecessary
+  // zero-extensions.
----------------
the -> they?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124894